[ENUM][CONFIG] Replaces all hard coded constant names in Enum#92
[ENUM][CONFIG] Replaces all hard coded constant names in Enum#92ChanukaUOJ merged 4 commits intoLDFLK:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors hardcoded constant names in the project to use Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively replaces hardcoded strings with enums, which is a great improvement for code clarity and maintainability. My review includes suggestions to further improve consistency in this new enum usage, particularly within the search service, and fixes a minor bug in the test data. Overall, this is a solid step towards a more robust codebase.
|
|
||
| # Check if we've reached a department, state minister or cabinet minister (root) | ||
| if current_category.kind and current_category.kind.minor in ["department", "stateMinister", "cabinetMinister"]: | ||
| if current_category.kind and current_category.kind.minor in [KindMinorEnum.DEPARTMENT.value, KindMinorEnum.STATE_MINISTER.value, KindMinorEnum.CABINET_MINISTER.value]: |
There was a problem hiding this comment.
For membership testing, using a set is more idiomatic and slightly more performant than a list. Since this list of minor kinds is also used on line 529, consider defining it as a constant set at the module level to improve readability and avoid repetition.
| if current_category.kind and current_category.kind.minor in [KindMinorEnum.DEPARTMENT.value, KindMinorEnum.STATE_MINISTER.value, KindMinorEnum.CABINET_MINISTER.value]: | |
| if current_category.kind and current_category.kind.minor in {KindMinorEnum.DEPARTMENT.value, KindMinorEnum.STATE_MINISTER.value, KindMinorEnum.CABINET_MINISTER.value}: |
| type_mapping = { | ||
| ("Organisation".lower(), "department".lower()): "department", | ||
| ("Organisation".lower(), "stateMinister".lower()): "stateMinister", | ||
| ("Organisation".lower(), "cabinetMinister".lower()): "cabinetMinister", | ||
| ("Dataset".lower(), "tabular".lower()): "dataset", | ||
| ("Person".lower(), "citizen".lower()): "person", | ||
| (KindMajorEnum.ORGANISATION.value.lower(), KindMinorEnum.DEPARTMENT.value.lower()): KindMinorEnum.DEPARTMENT.value, | ||
| (KindMajorEnum.ORGANISATION.value.lower(), KindMinorEnum.STATE_MINISTER.value.lower()): KindMinorEnum.STATE_MINISTER.value, | ||
| (KindMajorEnum.ORGANISATION.value.lower(), KindMinorEnum.CABINET_MINISTER.value.lower()): KindMinorEnum.CABINET_MINISTER.value, | ||
| (KindMajorEnum.DATASET.value.lower(), KindMinorEnum.TABULAR.value.lower()): "dataset", | ||
| (KindMajorEnum.PERSON.value.lower(), KindMinorEnum.CITIZEN.value.lower()): "person", | ||
| } |
There was a problem hiding this comment.
The type_mapping dictionary has inconsistent value types. It returns Enum members for Organisation kinds but hardcoded strings ('dataset', 'person') for others. This inconsistency propagates to VALID_ENTITY_TYPES (line 13) and entity_config (lines 54-60), undermining the goal of removing hardcoded strings.
To make this consistent, I suggest using Enum members for all types. This would involve:
- Updating this
type_mappingto returnKindMinorEnum.TABULAR.valueandKindMinorEnum.CITIZEN.value. - Updating
VALID_ENTITY_TYPESto use these enum values instead of hardcoded strings. - Updating the keys in
entity_configto also use these enum values.
| type_mapping = { | |
| ("Organisation".lower(), "department".lower()): "department", | |
| ("Organisation".lower(), "stateMinister".lower()): "stateMinister", | |
| ("Organisation".lower(), "cabinetMinister".lower()): "cabinetMinister", | |
| ("Dataset".lower(), "tabular".lower()): "dataset", | |
| ("Person".lower(), "citizen".lower()): "person", | |
| (KindMajorEnum.ORGANISATION.value.lower(), KindMinorEnum.DEPARTMENT.value.lower()): KindMinorEnum.DEPARTMENT.value, | |
| (KindMajorEnum.ORGANISATION.value.lower(), KindMinorEnum.STATE_MINISTER.value.lower()): KindMinorEnum.STATE_MINISTER.value, | |
| (KindMajorEnum.ORGANISATION.value.lower(), KindMinorEnum.CABINET_MINISTER.value.lower()): KindMinorEnum.CABINET_MINISTER.value, | |
| (KindMajorEnum.DATASET.value.lower(), KindMinorEnum.TABULAR.value.lower()): "dataset", | |
| (KindMajorEnum.PERSON.value.lower(), KindMinorEnum.CITIZEN.value.lower()): "person", | |
| } | |
| type_mapping = { | |
| (KindMajorEnum.ORGANISATION.value.lower(), KindMinorEnum.DEPARTMENT.value.lower()): KindMinorEnum.DEPARTMENT.value, | |
| (KindMajorEnum.ORGANISATION.value.lower(), KindMinorEnum.STATE_MINISTER.value.lower()): KindMinorEnum.STATE_MINISTER.value, | |
| (KindMajorEnum.ORGANISATION.value.lower(), KindMinorEnum.CABINET_MINISTER.value.lower()): KindMinorEnum.CABINET_MINISTER.value, | |
| (KindMajorEnum.DATASET.value.lower(), KindMinorEnum.TABULAR.value.lower()): KindMinorEnum.TABULAR.value, | |
| (KindMajorEnum.PERSON.value.lower(), KindMinorEnum.CITIZEN.value.lower()): KindMinorEnum.CITIZEN.value, | |
| } |
| @@ -1,3 +1,4 @@ | |||
| from typing import Optional | |||
There was a problem hiding this comment.
This import is not being used
Description
This PR addresses and completes Issue #56.
All previously hardcoded values related to Entity Major/Minor Kinds and Relation Names have been refactored to use
Enumtypes. This improves code readability, enhances maintainability, and allows safer and easier future modifications.Fixes #56
Changes
EnumreferencesEnumdefinitions for:Testing