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

Add Category:Swedish Cyprus Expedition to all non-categorized images #34

Merged
merged 3 commits into from May 4, 2017

Conversation

mattiasostmar
Copy link
Collaborator

@@ -367,8 +369,8 @@ def process_depicted_place(self, place_string, places_mapping):
place_as_wikitext = place_string

# Don't forget to add the commons categories, even though only wikidata is used in depicted people field
if places_mapping[place_string].get('commonscat'):
self.content_cats.append(places_mapping[place_string]["commonscat"])
if places_mapping[place_string].get('commonscat'):
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid making this change here since it is unrelated to what you want to do and will make this PR collide with the other patches.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, created new task not to forget it and reverted the change.


:return: None (populates self.content_cats)
"""
if not self.content_cats:
Copy link
Member

Choose a reason for hiding this comment

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

Do you also want to add a metacategory (needs more categories)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very true! Done!

""""
Check if there are any content cats added and add generic content category to image.

:return: None (populates self.content_cats)
Copy link
Member

Choose a reason for hiding this comment

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

Don't describe this as a return. You can just have populates self.content_cats as part of the docstring (on it's own line)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@lokal-profil lokal-profil left a comment

Choose a reason for hiding this comment

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

In addition to the in-line comment you also haven't reverted the change mentioned in #34 (comment)

@@ -435,6 +437,15 @@ def process_region_addition_to_description(region_str, country_str):

return region_addition

def add_catch_all_category(self, item):
""""
Populate self.content_cat with commons category, if present.
Copy link
Member

Choose a reason for hiding this comment

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

I meant that you could add this instead of the None I would still keep the original first line of the docstring though.

Also there should be no empty lines towards the end of the docstring (just one between the first line and the rest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, OK. Done.

@mattiasostmar mattiasostmar merged commit d1713f1 into master May 4, 2017
@lokal-profil lokal-profil deleted the catch_all_cat branch January 8, 2018 15:24
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.

None yet

2 participants