-
Notifications
You must be signed in to change notification settings - Fork 5
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
update to normaliser #66
Conversation
Codecov Report
@@ Coverage Diff @@
## main #66 +/- ##
==========================================
+ Coverage 98.89% 98.92% +0.02%
==========================================
Files 53 53
Lines 1901 1951 +50
==========================================
+ Hits 1880 1930 +50
Misses 21 21
Continue to review full report at Codecov.
|
filtered_df = filter_df_by_variables(df=mapping_df, form_number=filter_variables["form_number"], | ||
sf_notice_type=filter_variables["sf_notice_type"], | ||
legal_basis=filter_variables["legal_basis"], | ||
document_code=filter_variables["document_code"]) | ||
form_type = filtered_df["form_type"].values[0] |
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.
use constants instead raw strings
extracted_notice_type=extracted_notice_type, | ||
legal_basis=legal_basis, | ||
document_type_code=document_type_code) | ||
filtered_df = filter_df_by_variables(df=mapping_df, form_number=filter_variables["form_number"], |
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.
”form_number” use constants instead raw strings
value=self.normalise_legal_basis_value( | ||
extracted_metadata.legal_basis_directive | ||
)), | ||
"form_number": self.normalise_form_number(value=extracted_metadata.extracted_form_number), |
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.
use constants instead raw strings
document_type_code="7", | ||
legal_basis="legal") | ||
assert isinstance(filter_variables_dict,dict) | ||
assert filter_variables_dict["form_number"] == "F03" |
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.
use constants instead raw strings
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.
Well done!
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.
well done!
do not hard-coded so HARD the values which can be constants.
there is a typo (possibly) in the CSV
F05,,,,17 | ||
F06,,,,30 | ||
F07,,,O,15 |
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.
did we forget a 0
there ?
@@ -37,3 +37,6 @@ def sf_notice_df(self): | |||
def ef_notice_df(self): | |||
return get_mapping_csv_file("eforms_mapping.csv") | |||
|
|||
@property | |||
def filter_map_df(self): | |||
return get_mapping_csv_file("df_filter_map.csv") |
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.
the filename shall be a constant (light-coded), not a hardcoded value
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.
idem for other mappings
No description provided.