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

Refactor #1

Merged
merged 14 commits into from
Apr 12, 2023
Merged

Refactor #1

merged 14 commits into from
Apr 12, 2023

Conversation

boulund
Copy link
Member

@boulund boulund commented Apr 11, 2023

I took the liberty to go through the code and here suggest some structural improvements along with some minor refactors to make the code easier to read overall and potentially extend in the future.

Further future changes that I would recommend include:

  • Packaging as a Python package, for easy installation with pip, etc.
  • Packaging as a conda package
  • Using Python's logging module for info, warnings, and error messages instead of simply printing to stdout
  • Improve exception handling: the except clauses for the pd.read_csv calls for reading both VALENCIA and metadata input currently catches everything which is not considered best practice
  • Create a requirements.txt file to specify the minimum required version of Pandas needed to run VALODY (I think VALODY should work as intended on even very old versions of Pandas)

@boulund boulund requested a review from luhugerth April 11, 2023 14:30
@luhugerth
Copy link
Member

When the user does not input all necessary CST, the error message gives them in a random order like:

the following CSTs must be included: {'III-A', 'IV-B', 'III-B', 'IV-A', 'IV-C4', 'IV-C3', 'IV-C2', 'II-A', 'IV-C1', 'IV-C0', 'II-B', 'I-A', 'V', 'I-B'}

is it possible to make them come out alpha-numerically?

@luhugerth luhugerth merged commit 17cb300 into main Apr 12, 2023
@boulund
Copy link
Member Author

boulund commented Apr 12, 2023

The set data structure is naturally unordered, but of course it could be converted to a list and sorted before printing the error message if you want to, e.g.

            print(f"ERROR: When using subtypes, the following CSTs must be included: {sorted(ALL_SUBTYPE_CSTs)}")

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.

2 participants