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

Debug Archaeology Updater #512

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

fsimonjetz
Copy link
Contributor

No description provided.

Copy link

codeclimate bot commented Jan 29, 2024

Code Climate has analyzed commit 4c78a2b and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (75% is the threshold).

This pull request will bring the total coverage in the repository to 91.5% (0.0% change).

View more on Code Climate.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

PR Type: Refactoring

PR Summary: The pull request introduces a refactoring of the fragment update functionality within the repository and updater classes. It defines a new UpdatableField type and refactors the update methods to use this type, improving type safety and clarity. The PR also separates the transliteration update logic into its own method, further clarifying the codebase.

Decision: Comment

📝 Type: 'Refactoring' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.

General suggestions:

  • Ensure that the new update_transliteration method and the changes to update_field method are thoroughly tested, especially around the behavior when setting a field to None. This could potentially unset values in the database if not handled correctly.
  • Review the broader impact of changing the update_lemmatization method to use the 'text' field instead of 'lemmatization'. Confirm that this aligns with the intended database schema and that it won't lead to incorrect updates.
  • Consider the implications of the refactoring on existing code that may rely on the previous method signatures and ensure that all dependent code has been updated accordingly.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@fsimonjetz
Copy link
Contributor Author

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