Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Form proper labels for Category/Project update link #1422

Conversation

gromdimon
Copy link
Collaborator

Issue: #1421

@gromdimon gromdimon changed the base branch from main to dev April 29, 2024 16:25
@gromdimon gromdimon requested a review from mikkonie April 29, 2024 16:26
@gromdimon gromdimon self-assigned this Apr 29, 2024
@gromdimon gromdimon force-pushed the 1421-incorrect-project-type-strings-returned-for-sidebar-and-project-dropdown branch from dc9a33a to d37eb1f Compare May 2, 2024 10:21
projectroles/utils.py Outdated Show resolved Hide resolved
@@ -249,7 +249,7 @@ def get_project_app_links(
'projectroles:update',
kwargs={'project': project.sodar_uuid},
),
'label': 'Update Project',
'label': f'Update {get_display_name(project.type)}',
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to supply title=True as in the original implementation to ensure the name is returned capitalized. This was present in the original implementation. Please test these features to ensure they appear correct before submitting a pr.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm sorry, that I overlooked it! Now it should be fixed

projectroles/utils.py Show resolved Hide resolved
projectroles/tests/test_templatetags.py Outdated Show resolved Hide resolved
@gromdimon
Copy link
Collaborator Author

It looks like some 'label' lines are now too long. Although Black and Flake8 do not raise any issues, this definitely doesn't follow conventions. There are multiple ways to solve this problem: reformatting the string using parentheses (), using the '+' syntax, adding a helper method, or introducing some temporary variables before appending the link.

The question now is: how should I address this issue?

@gromdimon gromdimon force-pushed the 1421-incorrect-project-type-strings-returned-for-sidebar-and-project-dropdown branch from 7cc2fd3 to 4c81645 Compare May 6, 2024 12:00
@gromdimon gromdimon requested a review from mikkonie May 21, 2024 11:07
@mikkonie
Copy link
Collaborator

mikkonie commented Jun 4, 2024

It looks like some 'label' lines are now too long. Although Black and Flake8 do not raise any issues, this definitely doesn't follow conventions. There are multiple ways to solve this problem: reformatting the string using parentheses (), using the '+' syntax, adding a helper method, or introducing some temporary variables before appending the link.

The question now is: how should I address this issue?

This is a good point. You have "wrapped" the code inside the format string and strings are exempt from line length formatting in black. It is possible to do the following:

'label': f'Create '
'{get_display_name(PROJECT_TYPE_PROJECT, title=True)}',

That reduces line length and is accepted by black. Probably the best way unless you want to use the old style format() or get the title into a variable before inserting it into the string.

Copy link
Collaborator

@mikkonie mikkonie left a comment

Choose a reason for hiding this comment

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

I had one minor comment, plus I gave a hint on how to fix the line length issue in the pull request comments. Otherwise this looks fine and once these small things are fixed, I'll merge.

@@ -191,7 +191,7 @@ def get_project_app_links(
'projectroles:detail',
kwargs={'project': project.sodar_uuid},
),
'label': 'Project Overview',
'label': f'{get_display_name(project.type, title=True)} Overview',
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could get the string for the current project's type into a variable and use it both here and in the "Update Project" link. That would save us from calling the same method twice with identical input.

@gromdimon gromdimon requested a review from mikkonie June 9, 2024 10:01
@mikkonie mikkonie merged commit a0808d9 into dev Jun 11, 2024
5 checks passed
@mikkonie mikkonie deleted the 1421-incorrect-project-type-strings-returned-for-sidebar-and-project-dropdown branch June 11, 2024 07:00
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.

Incorrect project type strings returned for sidebar and project dropdown
2 participants