Skip to content

Conversation

@PouyaMohseni
Copy link
Contributor

@PouyaMohseni PouyaMohseni commented Nov 24, 2025

The list of labels on the instrument page is ordered by creation time. The label for the active language is prioritized by placing it at the top of the list.

refs: #410

Comment on lines 44 to 52
context["active_language"] = (
Language.objects.get(en_label=active_language_en)
if active_language_en
else Language.objects.get(en_label="English") # default in English
)

# Get the instrument label in the active language
# Get the instrument label in the active language and moving it to the top of label_aliases_dict
# Set label to the first instrument name added in the language if there is no "umil_label" set
active_labels = instrument_names.filter(language=context["active_language"])
active_lang = context["active_language"]
Copy link
Contributor

@dchiller dchiller Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than adding this to the context and then getting it, probably clearer to set the variable first and then add to context:

Suggested change
active_lang = context["active_language"]
active_lang = (
Language.objects.get(en_label=active_language_en)
if active_language_en
else Language.objects.get(en_label="English") # default in English
)
# Get the instrument label in the active language and moving it to the top of label_aliases_dict
# Set label to the first instrument name added in the language if there is no "umil_label" set
context["active_language"] = active_lang

@yinanazhou
Copy link
Member

I noticed this is still happening. If applicable, could you please reference the issue directly in the commit message following conventional commits, not only in the PR description? This will make it much more useful for future tracking and context.

@yinanazhou
Copy link
Member

yinanazhou commented Dec 5, 2025

There’s still room for improvement in the commit messages and the effort to keep the Git history clean is very much appreciated::

  • Please avoid using Git keywords (like resolves) if the change only partially addresses the issue. Merging the PR will auto-close the issue even though it’s not fully resolved. Following conventional commits, you can use refs: #xxxxx
  • Removing unused imports should be categorized as a chore.
  • Please make sure each commit message is concise and clear so people can understand what was done just from the message.

@yinanazhou
Copy link
Member

Minor improvements:

  • use refs in footers.
  • if you want to use 3 separate commits for this PR, the last one should be a refactor.

Otherwise, I believe this is good to go.

@PouyaMohseni
Copy link
Contributor Author

Thank you for your review and help.

In general, when the review is completed, should I squash my commits into a single one before merging, or should I keep them separate?

@yinanazhou
Copy link
Member

It depends. The author can decide based on what makes the codebase easier to understand and maintain for future developers.

@yinanazhou
Copy link
Member

The PR description still uses github keyword. And the first commit is not linking the related issue correctly. Otherwise, this looks good to me.

…page

- The active language's label is moved to the top of the label list so users see their expected language first.

refs: #410
- Cleaned up unused import that was accidentally left in the previous commit
Comment on lines 42 to 47
active_language_en = self.request.session.get("active_language_en", None)
context["active_language"] = (
active_lang = (
Language.objects.get(en_label=active_language_en)
if active_language_en
else Language.objects.get(en_label="English") # default in English
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
active_language_en = self.request.session.get("active_language_en", None)
context["active_language"] = (
active_lang = (
Language.objects.get(en_label=active_language_en)
if active_language_en
else Language.objects.get(en_label="English") # default in English
)
active_language_en = self.request.session.get("active_language_en", "English")
active_lang = Language.objects.get(en_label=active_language_en)

umil_label = active_labels.filter(umil_label=True)
if umil_label.exists():
context["active_instrument_label"] = umil_label.first()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if ordering your query of instrument names above wouldn't make all of this a lot simpler?

If you order by active language first, and then by umil_label = True, and then by date of creation (or pk), then I think your resulting queryset will be in the order you want, without all of this post-processing.

You could even go a step further and do the aggregation that is currently being done in python with your query, but at the very least, I think the ordering could be more clearly done in query.

@yinanazhou
Copy link
Member

I would suggest to highlight the first row by a different background color to show the user that this line is out of the order on purpose. But why are we ordering by creation time? Did we agree on order by alphabetical order?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants