Skip to content

fix: rename variable#1299

Merged
kpsherva merged 2 commits intoinveniosoftware:masterfrom
utnapischtim:community-vs-community-ui
May 8, 2025
Merged

fix: rename variable#1299
kpsherva merged 2 commits intoinveniosoftware:masterfrom
utnapischtim:community-vs-community-ui

Conversation

@utnapischtim
Copy link
Copy Markdown
Contributor

  • the variable name in communities_requests is community and not
    community_ui

@utnapischtim utnapischtim force-pushed the community-vs-community-ui branch from 602c778 to 2a5aa47 Compare May 4, 2025 21:10
"invenio_communities/details/requests/index.html",
theme=community_ui.get("theme", {}),
community=community_ui,
community_ui=community_ui,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Having the 2 vars with the same value is not super great. They have different purposes, see for example here.
I would suggest changing instead:

  • the current var in the previous line, which is clearly wrongly named:
- community=community_ui,
+ community_ui=community_ui,
  • its usage in the template here

WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the reason why i did the quick fix and add another variable with the same name and value was because i couldn't understand where community is used. since the refactor commit 9909924 introduced that bug, i thought it is safer to not remove community=community_ui

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so to be safe, all community variables would have to be renamed to community_ui in the context of invenio_communities/views/communities.py and the then used jinja templates?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the reason why i did the quick fix and add another variable with the same name and value was because i couldn't understand where community is used. since the refactor commit 9909924 introduced that bug, i thought it is safer to not remove community=community_ui

It should be in the view's template (I linked it above).

so to be safe, all community variables would have to be renamed to community_ui in the context of invenio_communities/views/communities.py and the then used jinja templates?

Only the ones that have value community_ui -> community=community_ui. Basically, community is the obj, and community_ui it serialized version, they are different. We should keep naming consistent to mark the difference.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Only the ones that have value community_ui -> community=community_ui. Basically, community is the obj, and community_ui it serialized version, they are different. We should keep naming consistent to mark the difference.

i think that doesn't work either without putting more thinking and testing time into it. for communities_subcommunities community is used here and here. can i replace that with community_ui["metadata"]["title"] and community_ui["slug"]?

btw this is also a problem here

so it could be that i need community=community and community_ui=community_ui, or i refactor the usage of community.metadata etc completly

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For the time being, fine by me adding both:

community=community
community_ui=community_ui

☺️

@utnapischtim utnapischtim force-pushed the community-vs-community-ui branch 2 times, most recently from 6ad15ce to 56effdb Compare May 5, 2025 11:53
* the variable name in communities_requests is community and not
  community_ui, so add community_ui=community_ui
@kpsherva kpsherva merged commit 316c6b1 into inveniosoftware:master May 8, 2025
4 of 6 checks passed
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.

3 participants