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
chore: tidy up excel2lists, excel2resources, excel2properties (DEV-1352) #229
Conversation
@@ -0,0 +1,54 @@ | |||
import dataclasses |
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.
To make excel2xml.py at least a bit shorter, I moved this class here.
@@ -236,30 +236,6 @@ def _make_json_lists_from_excel(excel_file_paths: list[str], verbose: bool = Fal | |||
return finished_lists | |||
|
|||
|
|||
def simplify_name(value: str) -> str: |
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.
moved to shared_methods.py
return True | ||
|
||
|
||
def prepare_dataframe(df: pd.DataFrame, required_columns: list[str], location_of_sheet: str) -> pd.DataFrame: |
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.
moved to shared_methods.py
@@ -25,20 +27,100 @@ def tearDownClass(cls) -> None: | |||
os.remove('testdata/tmp/' + file) | |||
os.rmdir('testdata/tmp') | |||
|
|||
def test_excel2jsonlist(self) -> None: | |||
|
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.
These 3 tests are new
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.
LGTM
I made some cosmetic remarks, but not much this time.
Regarding the issue with testing for exceptions, I think the way to solve this is to switch from the unittest
to the pytest
library. But it's up to you if you want to try that out in this PR or not.
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.
LGTM, also added some suggestions of minor importance.
resolves DEV-1352