-
Notifications
You must be signed in to change notification settings - Fork 4
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 AMPS #585
add AMPS #585
Conversation
Reviewer's Guide by SourceryThis PR adds support for the AMPS (Ancient Mesopotamian Priestly Scholasticism) research project by adding it as a new enum value in ResearchProject and updating related test files to include AMPS in their test cases. Class diagram for ResearchProject enum updateclassDiagram
class ResearchProject {
+CAIC: ("Cuneiform Artefacts of Iraq in Context", "CAIC", None)
+ALU_GENEVA: ("Edition of the Omen Series Summa Alu", "aluGeneva", None)
+AMPS: ("Ancient Mesopotamian Priestly Scholasticism", "AMPS", None)
}
note for ResearchProject "Added AMPS as a new enum value"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @zsomborfoldi - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Invalid syntax in ResearchProject access (link)
Overall Comments:
- In test_dtos.py, the syntax
ResearchProject["CAIC", "aluGeneva", "AMPS"].abbreviation
is incorrect. Each project's abbreviation should be accessed separately.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🔴 Testing: 1 blocking issue, 1 other issue
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -1084,11 +1084,12 @@ def test_query_genres(fragment_repository, query, expected): | |||
[ | |||
("CAIC", [0]), | |||
("aluGeneva", [1]), | |||
("AMPS", [2]), | |||
(None, [0, 1]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (testing): Test case for 'None' query should include all project indices
The test case for query=None should return [0, 1, 2] since we now have three projects in the test data. Currently, it's not testing the AMPS project in the null query case.
ebl/tests/fragmentarium/test_dtos.py
Outdated
@@ -113,7 +113,7 @@ def expected_dto(lemmatized_fragment, has_photo): | |||
"externalNumbers": ExternalNumbersSchema().dump( | |||
lemmatized_fragment.external_numbers | |||
), | |||
"projects": [ResearchProject["CAIC"].abbreviation], | |||
"projects": [ResearchProject["CAIC", "aluGeneva", "AMPS"].abbreviation], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (testing): Invalid syntax in ResearchProject access
The syntax for accessing ResearchProject enums is incorrect. It should be separate entries in a list like [ResearchProject.CAIC.abbreviation, ResearchProject.ALU_GENEVA.abbreviation, ResearchProject.AMPS.abbreviation]
Code Climate has analyzed commit 0a50558 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.4% (0.0% change). View more on Code Climate. |
Summary by Sourcery
Add the 'AMPS' project to the research projects and update related test cases to ensure proper integration.
New Features:
Tests: