[GrandComicsDatabase/gcd-django] Add character origin universe overview page with links to all (PR #696)
Adam Knights
As discussed, this adds a character origin universe overview page that completely mimics the other way of getting to these views, via level 3 characters. The page shows the same 8 views at the top such as Creators - the ones of those that have sub view links, such as the issue count on creators are also implemented. Additional names and main name links also work. Appearances with a story universe also work, so for example you can now find all the issues where Spider-Man [Miles Morales] originates from 1610 and appears in mainstream.
Testing has been alot, on both the new page, but also making sure the simple way and level 3 way continue to work.
image.png (view on web)Currently by design it is not showing all the other character stuff, as it is its own page, so for example i hide the editing menu on the right. Let me know if that's not ok / we should put a link back to the character not on the with origin view? - This should be minor either way.
The biggest improvement will be the 'without universe' overview page - these are views we couldn't access before via level 3.
There was also a minor bug where level 3 character features view didn't show issues, that's fixed. So https://www.comics.org/character/318/features/ should work after this is deployed.
In general as you read through the PR the pattern went as follows:
- Add the new url
- Find the equivalent existing method in 'details.py'
- Adjust that to take universe_id if it didn't already, protect active generalizations, and add code for without universe and heading
If the membership does vote to deprecate and remove level 3 characters, then i could go back and simplify away all the active generations ifs etc.
Last thing -> I think it makes sense to introduce these overview pages for groups - i'll do that once this PR in is.
You can view, comment on, or merge this pull request online at:
https://github.com/GrandComicsDatabase/gcd-django/pull/696
Commit Summary
- e7c96c4 Add character origin universe overview page with links to all
File Changes
(8 files)
- M apps/gcd/models/character.py (26)
- M apps/gcd/models/creator.py (52)
- M apps/gcd/models/feature.py (18)
- M apps/gcd/models/series.py (18)
- M apps/gcd/urls.py (36)
- M apps/gcd/views/details.py (722)
- M templates/gcd/details/tw_character.html (8)
- A templates/gcd/details/tw_character_origin_universe.html (95)
Patch Links:
- https://github.com/GrandComicsDatabase/gcd-django/pull/696.patch
- https://github.com/GrandComicsDatabase/gcd-django/pull/696.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
@adam-knights pushed 1 commit.
- fa1ecc7 Add character origin universe overview page with links to all
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
gemini-code-assist[bot]
@gemini-code-assist[bot] commented on this pull request.
Code Review
This pull request introduces a valuable new feature: a character origin universe overview page. This enhances data exploration by allowing users to view character information filtered by their origin universe. The implementation adds new URL routes and refactors several view functions in details.py to support this universe-based filtering, moving from static if/else blocks to more dynamic query building, which is a good improvement.
However, the refactoring has led to significant code duplication in details.py. I've identified a pattern of repeated logic for handling character/universe filtering and heading generation across multiple functions and suggested extracting it into a helper function to improve maintainability. I also found a minor bug in character_issues_series related to heading generation and some code duplication in creator.py that could be refactored. Addressing these points will make the codebase cleaner and more robust.
> +def character_characters(request, character_id, universe_id=None):
character = get_gcd_object(Character, character_id)
- universe_id = None
- if character.universe:
- if character.active_generalisations():
- filter_character = character.active_generalisations().get()\
- .from_character
- universe_id = character.universe.id
- else:
- filter_character = character
- else:
- filter_character = character
+ filter_character = character
+ link_universe_id = universe_id
+
+ if universe_id is None and character.universe and \
+ character.active_generalisations():
+ universe_id = character.universe.id
+ filter_character = character.active_generalisations().get()\
+ .from_character
query = {'character_names__storycharacter__story__'
'appearing_characters__character__character':
filter_character,
'character_names__storycharacter__deleted': False,
'character_names__storycharacter__story__type__id__in':
CORE_TYPES}
+
if universe_id:
- query['character_names__storycharacter__story__'
- 'appearing_characters__universe_id'] = universe_id
+ if universe_id == '-1':
+ universe_name = WITHOUT_UNIVERSE_NAME
+ else:
+ universe_obj = get_gcd_object(Universe, universe_id)
+ universe_name = universe_obj.universe_name()
+ query['character_names__storycharacter__story__'
+ 'appearing_characters__universe_id'] = universe_id
+
+ if link_universe_id:
+ heading = 'appearing together with %s with origin %s' % (
+ character, universe_name)
+ else:
+ heading = 'appearing together with %s' % (character)
characters = Character.objects.filter(Q(**query))\
.exclude(id=filter_character.id).distinct()
There is a large amount of duplicated code across many functions in this file (e.g., character_characters, character_features, character_issues, character_creators) for handling character filtering based on their universe and for generating dynamic page headings.
The following logic blocks are repeated in multiple functions:
- Determining
filter_characteranduniverse_idbased on character generalizations. - Building the query dictionary with conditions for
universe_id, including the special case for'-1'(without a universe). - Generating the
headingstring based on whether a universe was provided.
To improve maintainability and reduce redundancy, I strongly recommend extracting this common logic into a helper function. This function could accept the character, universe_id, and other relevant parameters, and return the filter_character, the final universe_id, the universe_name, and the base query dictionary. This would make the code much cleaner and easier to maintain.
In apps/gcd/models/creator.py:
> + def render_issue_count(self, record):
+ # Use character_origin_universe URL if universe_id is present
+ if self.resolve_name == 'character' and self.universe_id is not None:
+ url = urlresolvers.reverse(
+ 'character_origin_universe_creator_issues',
+ kwargs={'creator_id': record.id,
+ 'character_id': getattr(self, 'character').id,
+ 'universe_id': self.universe_id})
+ else:
+ url = urlresolvers.reverse(
+ '%s_creator_issues' % self.resolve_name,
+ kwargs={'creator_id': record.id,
+ '%s_id' % self.resolve_name:
+ getattr(self, self.resolve_name).id})
+ return mark_safe('<a href="%s">%s</a>' % (url, record.issue_count))
There's code duplication in the __init__ and render_issue_count methods across CreatorPortraitTable, GenericCreatorTable, and GenericCreatorNameTable. The logic for handling universe_id is very similar in these classes.
To improve maintainability, this duplicated logic should be refactored. Consider moving the common parts into the respective parent classes (CreatorTable and CreatorNameTable). This would centralize the logic and make future changes easier.
> + heading = 'for character %s with origin %s in %s' % ( + character, universe_name, series) +
There appears to be a bug here. The heading variable is set within the if universe_id: block, but it can be incorrectly overwritten by the subsequent if link_universe_id: block.
If link_universe_id is None but universe_id is determined from the character's generalization, the else branch of the second if statement will execute, overwriting the "with origin" heading with one that doesn't mention the origin.
This initial heading assignment seems redundant and is the source of the bug. Removing it should fix the issue.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
gemini-code-assist[bot]
@gemini-code-assist[bot] commented on this pull request.
Code Review
This pull request introduces a new character origin universe overview page, a significant feature that allows viewing character details filtered by their universe of origin. The implementation is thorough, involving new URL routing, extensive refactoring of view functions in details.py to accommodate a universe_id, and updates to table classes for correct link generation. The code is well-structured, and the refactoring of existing views to support this new feature is a good approach. My review includes a few suggestions for minor code cleanup and refactoring to further improve code quality and maintainability.
In apps/gcd/models/character.py:
> + universes = set(appearances.values_list('universe',
+ flat=True).distinct())
The use of set() here is redundant. The .distinct() method on a values_list queryset already ensures that the returned values are unique. Removing the set() call will make the code slightly more efficient and cleaner.
- universes = set(appearances.values_list('universe',
- flat=True).distinct())
+ universes = appearances.values_list('universe',
+ flat=True).distinct()
In apps/gcd/models/creator.py:
> + self.universe_id = kwargs.pop('universe_id', None)
super(CreatorPortraitTable, self).__init__(*args, **kwargs)
+ def render_issue_count(self, record):
+ # Use character_origin_universe URL if universe_id is present
+ if self.resolve_name == 'character' and self.universe_id is not None:
+ url = urlresolvers.reverse(
+ 'character_origin_universe_creator_issues',
+ kwargs={'creator_id': record.id,
+ 'character_id': getattr(self, 'character').id,
+ 'universe_id': self.universe_id})
+ else:
+ url = urlresolvers.reverse(
+ '%s_creator_issues' % self.resolve_name,
+ kwargs={'creator_id': record.id,
+ '%s_id' % self.resolve_name:
+ getattr(self, self.resolve_name).id})
+ return mark_safe('<a href="%s">%s</a>' % (url, record.issue_count))
There is significant code duplication here and in GenericCreatorTable and GenericCreatorNameTable. The logic for handling universe_id in __init__ and the implementation of render_issue_count are repeated. To improve maintainability, consider refactoring this logic into the respective base classes (CreatorTable and CreatorNameTable). This would involve moving the universe_id handling to the base __init__ and updating the base render_issue_count to include the new conditional logic.
> + heading = 'for character %s with origin %s in %s' % ( + character, universe_name, series)
This assignment to heading is redundant because its value is unconditionally reassigned in the following if link_universe_id: block. Removing this will make the code cleaner and prevent potential confusion.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JochenGCD
jochengcd left a comment (GrandComicsDatabase/gcd-django#696)Trying out gemini. The code duplication / helper function I also wondered about on the first glance. Arguably it could have been earlier as well, but now we have more code and logic than before in these places ?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
@adam-knights pushed 3 commits.
- 81a75c9 Fix heading bug and use of set
- 9e0ee50 Refactor creator.py
- dcd842d Introduce helper functions to reduce repeated code
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
@adam-knights commented on this pull request.
In apps/gcd/models/creator.py:
> + def render_issue_count(self, record):
+ # Use character_origin_universe URL if universe_id is present
+ if self.resolve_name == 'character' and self.universe_id is not None:
+ url = urlresolvers.reverse(
+ 'character_origin_universe_creator_issues',
+ kwargs={'creator_id': record.id,
+ 'character_id': getattr(self, 'character').id,
+ 'universe_id': self.universe_id})
+ else:
+ url = urlresolvers.reverse(
+ '%s_creator_issues' % self.resolve_name,
+ kwargs={'creator_id': record.id,
+ '%s_id' % self.resolve_name:
+ getattr(self, self.resolve_name).id})
+ return mark_safe('<a href="%s">%s</a>' % (url, record.issue_count))
Fixed in 9e0ee50
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
@adam-knights commented on this pull request.
> + heading = 'for character %s with origin %s in %s' % ( + character, universe_name, series) +
Fixed in 81a75c9
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
@adam-knights commented on this pull request.
In apps/gcd/models/character.py:
> + universes = set(appearances.values_list('universe',
+ flat=True).distinct())
Fixed in 81a75c9
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
@adam-knights commented on this pull request.
In apps/gcd/models/creator.py:
> + self.universe_id = kwargs.pop('universe_id', None)
super(CreatorPortraitTable, self).__init__(*args, **kwargs)
+ def render_issue_count(self, record):
+ # Use character_origin_universe URL if universe_id is present
+ if self.resolve_name == 'character' and self.universe_id is not None:
+ url = urlresolvers.reverse(
+ 'character_origin_universe_creator_issues',
+ kwargs={'creator_id': record.id,
+ 'character_id': getattr(self, 'character').id,
+ 'universe_id': self.universe_id})
+ else:
+ url = urlresolvers.reverse(
+ '%s_creator_issues' % self.resolve_name,
+ kwargs={'creator_id': record.id,
+ '%s_id' % self.resolve_name:
+ getattr(self, self.resolve_name).id})
+ return mark_safe('<a href="%s">%s</a>' % (url, record.issue_count))
Fixed in 9e0ee50
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
@adam-knights commented on this pull request.
> + heading = 'for character %s with origin %s in %s' % ( + character, universe_name, series)
Fixed in 81a75c9
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
> +def character_characters(request, character_id, universe_id=None):
character = get_gcd_object(Character, character_id)
- universe_id = None
- if character.universe:
- if character.active_generalisations():
- filter_character = character.active_generalisations().get()\
- .from_character
- universe_id = character.universe.idFixed in dcd842d
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
@adam-knights pushed 1 commit.
- e0700c9 Fix missing use of helper
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
adam-knights left a comment (GrandComicsDatabase/gcd-django#696)It's really nice! We should probably add an AGENTS.md file at root to help these things.
Was it free for open source?
I've addresses its comments, it spotted two minor bugs that would have been hard for a human to spot.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JochenGCD
jochengcd left a comment (GrandComicsDatabase/gcd-django#696)currently it is free for all
https://github.com/apps/gemini-code-assist
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
gemini-code-assist[bot]
gemini-code-assist[bot] left a comment (GrandComicsDatabase/gcd-django#696)Thanks, @jochengcd, for clarifying the pricing! And @adam-knights, I'm glad I could help spot those bugs. Gemini Code Assist is indeed currently free for all users, and we're always working to improve its capabilities.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
gemini-code-assist[bot]
@gemini-code-assist[bot] commented on this pull request.
Code Review
This pull request introduces a new character origin universe overview page, which is a significant feature addition. The implementation involves adding new URLs, a new view, and a corresponding template. A substantial part of the work is the refactoring of existing views and model table classes to accommodate universe-based filtering. The refactoring in details.py, particularly the extraction of duplicated logic into helper functions, is a commendable improvement to the codebase's maintainability. My review includes suggestions to further reduce code duplication in some of the model table classes. I've also identified a potential server error on the new page that could occur if a character lacks an official name and have proposed a fix to handle this gracefully.
> + context = {
+ 'character': character,
+ 'additional_names': character.active_names()
+ .filter(is_official_name=False),
+ 'universe': universe,
+ 'universe_id': universe_id,
+ 'universe_name': universe_name,
+ 'image_tag': image_tag,
+ 'image_issue': selected_issue,
+ 'active_universe_appearances':
+ character.active_universe_appearances_with_origin(universe),
+ 'error_subject': '%s - %s' % (character, universe_name)
+ }
The template tw_character_origin_universe.html directly accesses character.official_name.id. The official_name() method on the Character model uses .get(), which will raise a DoesNotExist exception if no official name is found, causing a server error. It's safer to handle this potential exception in the view by fetching the official name within a try-except block and passing it to the template context. This makes the view more robust.
- context = {
- 'character': character,
- 'additional_names': character.active_names()
- .filter(is_official_name=False),
- 'universe': universe,
- 'universe_id': universe_id,
- 'universe_name': universe_name,
- 'image_tag': image_tag,
- 'image_issue': selected_issue,
- 'active_universe_appearances':
- character.active_universe_appearances_with_origin(universe),
- 'error_subject': '%s - %s' % (character, universe_name)
- }
+ try:
+ official_name = character.official_name()
+ except CharacterNameDetail.DoesNotExist:
+ # It's good practice to log this, as it indicates a data integrity issue.
+ official_name = None
+
+ context = {
+ 'character': character,
+ 'official_name': official_name,
+ 'additional_names': character.active_names()
+ .filter(is_official_name=False),
+ 'universe': universe,
+ 'universe_id': universe_id,
+ 'universe_name': universe_name,
+ 'image_tag': image_tag,
+ 'image_issue': selected_issue,
+ 'active_universe_appearances':
+ character.active_universe_appearances_with_origin(universe),
+ 'error_subject': '%s - %s' % (character, universe_name)
+ }
In templates/gcd/details/tw_character_origin_universe.html:
> + </tr>
+{% endif %}
+ </table>
+ <ul class="object-page-link-list">
+ <li><a href='{% url "character_origin_universe_creators" character_id=character.id universe_id=universe_id %}'>Creator List</a></li>
+ <li><a href='{% url "character_origin_universe_creator_names" character_id=character.id universe_id=universe_id %}'>Creator Name List</a></li>
+ <li><a href='{% url "character_origin_universe_issues" character_id=character.id universe_id=universe_id %}'>Issue List</a></li>
+ <li><a href='{% url "character_origin_universe_covers" character_id=character.id universe_id=universe_id %}'>Cover List</a></li>
+ <li><a href='{% url "character_origin_universe_series" character_id=character.id universe_id=universe_id %}'>Series List</a></li>
+ <li><a href='{% url "character_origin_universe_features" character_id=character.id universe_id=universe_id %}'>Feature List</a></li>
+ <li><a href='{% url "character_origin_universe_characters" character_id=character.id universe_id=universe_id %}'>Characters Appeared With</a></li>
+ <li><a href='{% url "character_origin_universe_sequences" character_id=character.id universe_id=universe_id %}'>Sequences List</a></li>
+ </ul>
+ <h3>Name:</h3>
+ <ol class="ps-4">
+ <a href='{% url "character_name_origin_universe_issues" character_name_id=character.official_name.id universe_id=universe_id %}'>{{ character.official_name.name }}</a>
This line directly accesses character.official_name.id, which can cause a server error if a character has no official name. To prevent this, you should use the official_name variable passed from the view (as suggested in my other comment) and check if it exists before trying to access its properties. This makes the template more resilient to data inconsistencies.
{% if official_name %}
<a href='{% url "character_name_origin_universe_issues" character_name_id=official_name.id universe_id=universe_id %}'>{{ official_name.name }}</a>
{% else %}
[No official name available]
{% endif %}
In apps/gcd/models/creator.py:
> def render_issue_count(self, record):
- url = urlresolvers.reverse(
- '%s_creator_issues' % self.resolve_name,
+ # Use character_origin_universe URL if universe_id is present
+ if self.resolve_name == 'character' and self.universe_id is not None:
+ url = urlresolvers.reverse(
+ 'character_origin_universe_creator_issues',
kwargs={'creator_id': record.id,
- '%s_id' % self.resolve_name:
- getattr(self, self.resolve_name).id})
+ 'character_id': getattr(self, 'character').id,
+ 'universe_id': self.universe_id})
+ else:
+ url = urlresolvers.reverse(
+ '%s_creator_issues' % self.resolve_name,
+ kwargs={'creator_id': record.id,
+ '%s_id' % self.resolve_name:
+ getattr(self, self.resolve_name).id})
return mark_safe('<a href="%s">%s</a>' % (url, record.issue_count))
To improve maintainability and reduce code duplication, you can refactor this method to extract the URL name and keyword arguments into local variables before calling urlresolvers.reverse. This makes the code cleaner and easier to read.
- def render_issue_count(self, record):
- url = urlresolvers.reverse(
- '%s_creator_issues' % self.resolve_name,
- # Use character_origin_universe URL if universe_id is present
- if self.resolve_name == 'character' and self.universe_id is not None:
- url = urlresolvers.reverse(
- 'character_origin_universe_creator_issues',
- kwargs={'creator_id': record.id,
- '%s_id' % self.resolve_name:
- getattr(self, self.resolve_name).id})
- 'character_id': getattr(self, 'character').id,
- 'universe_id': self.universe_id})
- else:
- url = urlresolvers.reverse(
- '%s_creator_issues' % self.resolve_name,
- kwargs={'creator_id': record.id,
- '%s_id' % self.resolve_name:
- getattr(self, self.resolve_name).id})
- return mark_safe('<a href="%s">%s</a>' % (url, record.issue_count))
+ def render_issue_count(self, record):
+ # Use character_origin_universe URL if universe_id is present
+ if self.resolve_name == 'character' and self.universe_id is not None:
+ url_name = 'character_origin_universe_creator_issues'
+ kwargs = {'creator_id': record.id,
+ 'character_id': getattr(self, 'character').id,
+ 'universe_id': self.universe_id}
+ else:
+ url_name = '%s_creator_issues' % self.resolve_name
+ kwargs = {'creator_id': record.id,
+ '%s_id' % self.resolve_name:
+ getattr(self, self.resolve_name).id}
+ url = urlresolvers.reverse(url_name, kwargs=kwargs)
+ return mark_safe('<a href="%s">%s</a>' % (url, record.issue_count))
In apps/gcd/models/creator.py:
> def render_issue_count(self, record):
- url = urlresolvers.reverse(
- '%s_creator_name_issues' % self.resolve_name,
+ # Use character_origin_universe URL if universe_id is present
+ if self.resolve_name == 'character' and self.universe_id is not None:
+ url = urlresolvers.reverse(
+ 'character_origin_universe_creator_name_issues',
kwargs={'creator_name_id': record.id,
- '%s_id' % self.resolve_name:
- getattr(self, self.resolve_name).id})
+ 'character_id': getattr(self, 'character').id,
+ 'universe_id': self.universe_id})
+ else:
+ url = urlresolvers.reverse(
+ '%s_creator_name_issues' % self.resolve_name,
+ kwargs={'creator_name_id': record.id,
+ '%s_id' % self.resolve_name:
+ getattr(self, self.resolve_name).id})
return mark_safe('<a href="%s">%s</a>' % (url, record.issue_count))
Similar to the render_issue_count method in the parent CreatorTable, this method can be refactored to improve readability and reduce code duplication. By defining the URL name and keyword arguments in local variables, the structure of the method becomes cleaner.
- def render_issue_count(self, record):
- url = urlresolvers.reverse(
- '%s_creator_name_issues' % self.resolve_name,
- # Use character_origin_universe URL if universe_id is present
- if self.resolve_name == 'character' and self.universe_id is not None:
- url = urlresolvers.reverse(
- 'character_origin_universe_creator_name_issues',
- kwargs={'creator_name_id': record.id,
- '%s_id' % self.resolve_name:
- getattr(self, self.resolve_name).id})
- 'character_id': getattr(self, 'character').id,
- 'universe_id': self.universe_id})
- else:
- url = urlresolvers.reverse(
- '%s_creator_name_issues' % self.resolve_name,
- kwargs={'creator_name_id': record.id,
- '%s_id' % self.resolve_name:
- getattr(self, self.resolve_name).id})
- return mark_safe('<a href="%s">%s</a>' % (url, record.issue_count))
+ def render_issue_count(self, record):
+ # Use character_origin_universe URL if universe_id is present
+ if self.resolve_name == 'character' and self.universe_id is not None:
+ url_name = 'character_origin_universe_creator_name_issues'
+ kwargs = {'creator_name_id': record.id,
+ 'character_id': getattr(self, 'character').id,
+ 'universe_id': self.universe_id}
+ else:
+ url_name = '%s_creator_name_issues' % self.resolve_name
+ kwargs = {'creator_name_id': record.id,
+ '%s_id' % self.resolve_name:
+ getattr(self, self.resolve_name).id}
+ url = urlresolvers.reverse(url_name, kwargs=kwargs)
+ return mark_safe('<a href="%s">%s</a>' % (url, record.issue_count))
In apps/gcd/models/feature.py:
> def render_issue_count(self, record):
- url = urlresolvers.reverse(
- '%s_issues_per_feature' % self.resolve_name,
- kwargs={'feature_id': record.id,
- '%s_id' % self.resolve_name:
- getattr(self, self.resolve_name).id})
+ if self.resolve_name == 'character' and self.universe_id is not None:
+ url = urlresolvers.reverse(
+ 'character_origin_universe_issues_per_feature',
+ kwargs={'feature_id': record.id,
+ 'character_id': self.character.id,
+ 'universe_id': self.universe_id})
+ else:
+ url = urlresolvers.reverse(
+ '%s_issues_per_feature' % self.resolve_name,
+ kwargs={'feature_id': record.id,
+ '%s_id' % self.resolve_name:
+ getattr(self, self.resolve_name).id})
return mark_safe('<a href="%s">%s</a>' % (url,
record.issue_count))
This method contains duplicated calls to urlresolvers.reverse. To make the code more concise and maintainable, you can extract the URL name and arguments into variables before making the call.
def render_issue_count(self, record):
if self.resolve_name == 'character' and self.universe_id is not None
:
url_name = 'character_origin_universe_issues_per_feature'
kwargs = {'feature_id': record.id,
'character_id': self.character.id,
'universe_id': self.universe_id}
else:
url_name = '%s_issues_per_feature' % self.resolve_name
kwargs = {'feature_id': record.id,
'%s_id' % self.resolve_name:
getattr(self, self.resolve_name).id}
url = urlresolvers.reverse(url_name, kwargs=kwargs)
return mark_safe('<a href="%s">%s</a>' % (url, record.issue_count))
> def render_appearances_count(self, record):
- url = urlresolvers.reverse(
- '%s_issues_per_series' % self.resolve_name,
- kwargs={'series_id': record.id,
- '%s_id' % self.resolve_name:
- getattr(self, self.resolve_name).id})
+ if self.resolve_name == 'character' and self.universe_id is not None:
+ url = urlresolvers.reverse(
+ 'character_origin_universe_issues_per_series',
+ kwargs={'series_id': record.id,
+ 'character_id': self.character.id,
+ 'universe_id': self.universe_id})
+ else:
+ url = urlresolvers.reverse(
+ '%s_issues_per_series' % self.resolve_name,
+ kwargs={'series_id': record.id,
+ '%s_id' % self.resolve_name:
+ getattr(self, self.resolve_name).id})
return mark_safe('<a href="%s">%s</a>' % (url,
record.appearances_count))
To improve code clarity and maintainability, you can refactor this method by extracting the URL name and keyword arguments into variables. This avoids duplicating the urlresolvers.reverse call.
def render_appearances_count(self, record):
if self.resolve_name == 'character' and self.universe_id is not None
:
url_name = 'character_origin_universe_issues_per_series'
kwargs = {'series_id': record.id,
'character_id': self.character.id,
'universe_id': self.universe_id}
else:
url_name = '%s_issues_per_series' % self.resolve_name
kwargs = {'series_id': record.id,
'%s_id' % self.resolve_name:
getattr(self, self.resolve_name).id}
url = urlresolvers.reverse(url_name, kwargs=kwargs)
return mark_safe('<a href="%s">%s</a>' % (url
,
record.appearances_count))—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JochenGCD
@jochengcd commented on this pull request.
some questions on formatting, in the end minor, but we should aim to be consistent to the current practise
> @@ -5359,9 +5494,10 @@ def credit_type_history(request, story_id, credit_type):
raise ValueError
story = get_object_or_404(Story, id=story_id)
- storyrevisions = story.revisions.filter(changeset__state=states.APPROVED)\
- .order_by('modified', 'id')\
- .select_related('changeset__indexer')
+ storyrevisions = (story.revisions
Why touch formatting of otherwise unchanged code ?
In apps/gcd/models/creator.py:
> @@ -246,7 +246,7 @@ def display_credit(self, credit, url=True, compare=False, search=False,
attribute = 'under joint name '
display_as_name = as_name.name
elif self.type_id == NAME_TYPES['misspelled'] and \
- credit.is_credited:
+ credit.is_credited:
why change ?
> filter_character_with = character_with.active_generalisations()\ - .get().from_character + .get().from_character
again formatting, we try to keep it at the end of the line to have the logic is fine. and it passes flake8, so no need to change ?
> - story__appearing_characters__deleted=False).distinct()
- heading = 'for creator %s for character %s' % (creator,
- character)
+ filter_character, universe_id, link_universe_id = \
+ _resolve_character_universe(character, universe_id)
+
+ query = {
+ 'story__credits__creator__creator': creator,
+ 'story__appearing_characters__character__character':
+ filter_character,
+ 'story__appearing_characters__deleted': False,
+ 'story__type__id__in': story_types
+ }
+
+ heading = _build_universe_filter_and_heading(
+ universe_id, link_universe_id, query,
I don't remember why we did it, but as the intendation after a ( we use two spaces and not four.
This would be in several places.
Minor, feel free to leave it is this time, but please keep the existing style in the future.
> @@ -5370,7 +5506,8 @@ def credit_type_history(request, story_id, credit_type):
changed = old.strip() != new.strip()
if not changed:
credits = storyrev.story_credit_revisions.filter(
- credit_type__name=credit_type)
+ credit_type__name=credit_type
flake8 didn't complain here before, more consistent to out practice would to add a space to be aligned to story_credit_revisions
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
adam-knights left a comment (GrandComicsDatabase/gcd-django#696)Interesting - so flake8 was complaining on mine, that was why i fixed it as i was struggling to see what ones i'd introduced. Flake8 isn't locked in requirements, so it's installed 7.3.0 for me.. i'll check this later and report back :).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JochenGCD
jochengcd left a comment (GrandComicsDatabase/gcd-django#696)for me
flake8 --version
6.1.0 (mccabe: 0.7.0, pycodestyle: 2.12.1, pyflakes: 3.1.0) CPython 3.14.2 on Linux
But I would wonder a bit, if it now would complain about some of these ?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JochenGCD
jochengcd left a comment (GrandComicsDatabase/gcd-django#696)The Character at my.comics.org-link would be useful. So one can see if there are issues in a collection with the character from that universe.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
@adam-knights pushed 1 commit.
- 1ac8555 Fix formatting and pin flake8
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
adam-knights left a comment (GrandComicsDatabase/gcd-django#696)Yes it was the flake8 version difference, i've pinned the version along with fixed them back, so then in future if we goto 7 then we can fix any new over-indent issues with it as a choice.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
adam-knights left a comment (GrandComicsDatabase/gcd-django#696)The Character at my.comics.org-link would be useful. So one can see if there are issues in a collection with the character from that universe.
I'm not so familiar with this - where do you mean? In this PR or a separate addition?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
@adam-knights commented on this pull request.
> - story__appearing_characters__deleted=False).distinct()
- heading = 'for creator %s for character %s' % (creator,
- character)
+ filter_character, universe_id, link_universe_id = \
+ _resolve_character_universe(character, universe_id)
+
+ query = {
+ 'story__credits__creator__creator': creator,
+ 'story__appearing_characters__character__character':
+ filter_character,
+ 'story__appearing_characters__deleted': False,
+ 'story__type__id__in': story_types
+ }
+
+ heading = _build_universe_filter_and_heading(
+ universe_id, link_universe_id, query,
I see yes - we might be able to do something like this with a flake8 setting, i.e. continuation-indent-width
We could try adding a .flake8 file with:
[flake8]
continuation-indent-width = 2
But probably better to do in one go to pickup all at once, when neither has big potential code coming in for conflicts?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JochenGCD
jochengcd left a comment (GrandComicsDatabase/gcd-django#696)The Character at my.comics.org-link would be useful. So one can see if there are issues in a collection with the character from that universe.
I'm not so familiar with this - where do you mean? In this PR or a separate addition?
on the character page we have
{% block www_my_comics_cross_link %}
{% url "show_character" character_id=character.id as object_url %}
{% with model_name='Character' object_url=object_url %}
{{ block.super }}
{% endwith %}
{% endblock %}
here
{% url "show_character" character_id=character.id as object_url %}
show be the url of the current page, the block.super handles the rest
The filtering by collection should work automatically by the enhanced filter on my.comics.org
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
@adam-knights pushed 1 commit.
- 05dceb4 Add missing my comics block to origin page
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
adam-knights left a comment (GrandComicsDatabase/gcd-django#696)The Character at my.comics.org-link would be useful. So one can see if there are issues in a collection with the character from that universe.
I'm not so familiar with this - where do you mean? In this PR or a separate addition?
on the character page we have {% block www_my_comics_cross_link %} {% url "show_character" character_id=character.id as object_url %} {% with model_name='Character' object_url=object_url %} {{ block.super }} {% endwith %} {% endblock %} here {% url "show_character" character_id=character.id as object_url %} show be the url of the current page, the block.super handles the rest
The filtering by collection should work automatically by the enhanced filter on my.comics.org
Got it! Its one of those buttons my brain just didnt see any more! Added it back :).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JochenGCD
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
JochenGCD
jochengcd left a comment (GrandComicsDatabase/gcd-django#696)missed a page, this one fails
https://www.comics.org/universe/78/characters/
NoReverseMatch at /universe/78/characters/
Reverse for 'character_issues_per_universe' not found. 'character_issues_per_universe' is not a valid view function or pattern name.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
adam-knights left a comment (GrandComicsDatabase/gcd-django#696)Will work on a fix now..
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Knights
adam-knights left a comment (GrandComicsDatabase/gcd-django#696)Fix up in #697
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.