Skip to content

Conversation

@jbh1996
Copy link

@jbh1996 jbh1996 commented May 20, 2025

This pull request creates the copyright class in the wiki client.py and updates the get_image function to populate it. It also edits the Image class to include a copyright attribute.

@bartfeenstra
Copy link
Owner

bartfeenstra commented May 21, 2025

@jbh1996 Thanks for your PR! As you can see, some tests are failing. You can run these locally with bin/test. Most static check fails can usually be fixed automatically by running bin/fix. This is a good idea to do regularly during development as it informs you of errors that are often easy to fix, such as type errors or code style violations.

What's the scope of this PR? To display this copyright information on the file pages? E.g. to fix #2583 ?

@bartfeenstra
Copy link
Owner

@jbh1996 I noticed your Betty fork was made from Betty 0.4.x. Since then, the 0.5.x branch was created and all new development happens there (with bug fixes potentially backported to 0.4.x once fixed in 0.5.x). You'll want to make sure your local development branch is kept up to date with 0.5.x, or it'll end up diverging so much your Git changes can no longer be applied (preventing a PR merge). Let me know if you need help with something 👍

@jbh1996
Copy link
Author

jbh1996 commented May 28, 2025

Hey Bart! I just got a chance to work on this again. I made some edits and the bin/fix is giving me no errors back. Let me know if there's still changes that need to be made to the pull request. Sorry this took me a bit to get to.

@bartfeenstra
Copy link
Owner

@jbh1996 No worries, and take your time. Thanks for addressing the feedback and fixing the test failures. Your code looks good but we're now at the point where we need to do something with this new functionality, as well as write tests for it. In order to determine exactly what else needs to be done, is it #2583 you are trying to fix? If so (please confirm in a comment), then what remains is:

  • To create a new copyright plugin that lets Betty assign this newly fetched data to actual file entities. Place the new plugin in betty.wiki.copyright_notice, along with the existing one.
  • Update betty.wiki.populator.Populator._image_file_reference(), which is the function responsible for converting the data returned by the Wikipedia Client to entities for in the data model. You'll see it currently already sets a copyright notice, but that's the wrong one.
  • Because we no longer need betty.wiki.populator.Populator._copyright_notice, the entire attribute may be removed, the corresponding argument removed from __init__(), and calling code updated so it no longer provides this argument.

@jbh1996
Copy link
Author

jbh1996 commented May 29, 2025

Yes it is #2583 that I'm trying to fix. I'll get into those files in the next few days to try to make this work. Thanks for your patience and pointers.

@bartfeenstra bartfeenstra changed the title Copyright Class Creation Fix incorrect copyright notice for Wikimedia images May 29, 2025
@bartfeenstra bartfeenstra added python Pull requests that update Python code bug Something isn't working labels Jun 2, 2025
@jbh1996
Copy link
Author

jbh1996 commented Jun 3, 2025

I've made changes to both the copyright_notice and populator file for your review!

Copy link
Owner

@bartfeenstra bartfeenstra left a comment

Choose a reason for hiding this comment

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

Please have a look at the failing tests. For fastest feedback, run the tests locally on your own machine before pushing, so you can fix failures quickly without having to wait for the Github builds to complete.

title: str
wikimedia_commons_url: str
name: str
image_copyright: Copyright | None
Copy link
Owner

Choose a reason for hiding this comment

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

The class is called "image" already, so we can simply call the attribute "copyright".

url=image.image_copyright.url,
)
if image.image_copyright
else await WikipediaContributors.new_for_app(self._ancestry.app)
Copy link
Owner

Choose a reason for hiding this comment

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

The original issue this PR tries to fix is that we are currently telling people these images are copyright Wikipedia contributors, but that is not the case (the images do not even come from Wikipedia). If we cannot get copyright information from Wikimedia, we should therefore let the image copyright be empty.

links=links,
copyright_notice=self._copyright_notice,
copyright_notice=(
await WikimediaCommonsCopyright.new_for_app(
Copy link
Owner

Choose a reason for hiding this comment

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

These factory methods exist so that code can instantiate classes without knowing what arguments their __init__() methods require, usually because the class name is not hardcoded. In this case we know the class name, so we can instantiate it without having to call .new_for_app().

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

Labels

bug Something isn't working python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants