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

chore: tidy up excel2lists, excel2resources, excel2properties (DEV-1352) #229

Merged
merged 8 commits into from Sep 19, 2022

Conversation

jnussbaum
Copy link
Collaborator

resolves DEV-1352

@jnussbaum jnussbaum self-assigned this Sep 16, 2022
@@ -0,0 +1,54 @@
import dataclasses
Copy link
Collaborator Author

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:
Copy link
Collaborator Author

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:
Copy link
Collaborator Author

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:

Copy link
Collaborator Author

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

Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a 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.

knora/dsplib/models/propertyelement.py Outdated Show resolved Hide resolved
knora/dsplib/models/propertyelement.py Show resolved Hide resolved
knora/dsplib/utils/excel_to_json_lists.py Outdated Show resolved Hide resolved
knora/dsplib/models/propertyelement.py Show resolved Hide resolved
knora/dsplib/utils/shared_methods.py Show resolved Hide resolved
knora/dsplib/utils/xml_upload.py Outdated Show resolved Hide resolved
knora/excel2xml.py Show resolved Hide resolved
test/unittests/test_excel_to_json_lists.py Show resolved Hide resolved
Copy link

@irinaschubert irinaschubert left a 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.

knora/dsplib/models/propertyelement.py Show resolved Hide resolved
knora/dsplib/models/propertyelement.py Show resolved Hide resolved
knora/dsplib/models/propertyelement.py Show resolved Hide resolved
@jnussbaum jnussbaum merged commit d2c2e08 into main Sep 19, 2022
@jnussbaum jnussbaum deleted the wip/dev-1352-tidy-up branch September 19, 2022 15:22
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.

None yet

3 participants