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

Fixes for creator field #76

Merged
merged 1 commit into from
Jan 8, 2025
Merged

Fixes for creator field #76

merged 1 commit into from
Jan 8, 2025

Conversation

mdellabitta
Copy link
Contributor

@mdellabitta mdellabitta commented Jan 8, 2025

Made markup different per https://commons.wikimedia.org/wiki/User_talk:Dominic#DPLA_bot_use_of_InFi_for_creator_field

Omit creator when missing.


Important

Update get_wiki_text to conditionally include creator field based on presence, with corresponding test updates.

  • Behavior:
    • Update get_wiki_text in wikimedia.py to conditionally include creator field in template only if present.
    • If creator is missing, omit the creator field from the template.
  • Tests:
    • Update test_get_wiki_text in test_wikimedia.py to verify behavior with and without creator field.

This description was created by Ellipsis for f2ebcbc. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to f2ebcbc in 42 seconds

More details
  • Looked at 68 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. tests/test_wikimedia.py:163
  • Draft comment:
    The test for get_wiki_text without a creator should explicitly verify that the creator field is omitted from the output. Consider adding an assertion to check that the creator field is not present in text.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The current test is already comprehensive - it checks the exact text output character by character. If the creator field was present when it shouldn't be, the test would fail. An additional assertion would be redundant since we're already doing an exact string match. The suggestion would add test complexity without adding value.
    Perhaps an explicit assertion about the creator field would make the test's intention clearer to future readers? It could serve as documentation of what we're testing.
    While explicit assertions can be helpful for documentation, in this case the test is already clear and the expected_text clearly shows the intended structure without the creator field. Adding redundant assertions goes against testing best practices.
    Delete this comment because the existing exact string comparison test already thoroughly verifies the absence of the creator field. Adding another assertion would be redundant.
2. ingest_wikimedia/wikimedia.py:141
  • Draft comment:
    Ensure no hardcoded API keys are present. Use environment variables or secure vaults for storing API keys.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The code does not seem to have any hardcoded API keys, which is the only rule violation I am checking for.

Workflow ID: wflow_UoUYB7DVKHBBRvHW


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@mdellabitta mdellabitta merged commit 4cfad90 into main Jan 8, 2025
7 checks passed
@mdellabitta mdellabitta deleted the fix-creator-template branch January 8, 2025 19:27
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.

1 participant