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: parameter license_classifier expects a list #133

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

b-kamphorst
Copy link
Contributor

Alternative solution to #132. This solution ensures that parameter license_classifier to select_license_by_source is a list, resulting in code that is easier to reason about.

@b-kamphorst
Copy link
Contributor Author

I actually intended to write a test that captures the issue that #132 (comment) describes, but this seemed simpler.

In general I find the logic is challenging to test. @raimon49 I might be willing to spend some time to (rigorously) refactor the code in some weeks if you are interested, but only if you are (1) open to that and (2) willing to spend time to think along. For example, I wonder whether FIELDS_TO_METADATA_KEYS is still relevant, I think that type annotations would help in further refactoring and my main aim is to make the code more testable (e.g. not depend on installed dependencies). How would you feel about that?

This prevents multiple branches in select_license_by_source
@codecov
Copy link

codecov bot commented Oct 30, 2022

Codecov Report

Merging #133 (ccc4790) into release-4.0.0 (0c1efdd) will not change coverage.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           release-4.0.0      #133   +/-   ##
===============================================
  Coverage         100.00%   100.00%           
===============================================
  Files                  1         1           
  Lines                374       371    -3     
===============================================
- Hits                 374       371    -3     
Impacted Files Coverage Δ
piplicenses.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Comment on lines -169 to +171
classifiers = metadata.get_all("classifier")
if classifiers:
pkg_info['license_classifier'] = \
find_license_from_classifier(classifiers)
classifiers = metadata.get_all("classifier", [])
pkg_info['license_classifier'] = \
find_license_from_classifier(classifiers)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have checked API Reference of importlib_metadata.

It's a simple and beautiful solution 👍

@raimon49 raimon49 merged commit 3b7f732 into raimon49:release-4.0.0 Oct 31, 2022
@raimon49
Copy link
Owner

In general I find the logic is challenging to test. @raimon49 I might be willing to spend some time to (rigorously) refactor the code in some weeks if you are interested, but only if you are (1) open to that and (2) willing to spend time to think along. For example, I wonder whether FIELDS_TO_METADATA_KEYS is still relevant, I think that type annotations would help in further refactoring and my main aim is to make the code more testable (e.g. not depend on installed dependencies). How would you feel about that?

Refactoring to make it testable is a "nice to have."

Perhaps this can be done by proceeding in small scopes of separation. A huge release may not move forward due to my lack of time or ability to do so.

e.g.) After v4.0.0 shipped,

  • Introduce type annotations -> Ships next release
  • Tests not depend on installed dependencies -> Ships next release
  • And to more testable code...

We cannot decide here and now what order to work in, but small separated patches are acceptable.

@b-kamphorst b-kamphorst deleted the release-4.0.0 branch October 31, 2022 20:03
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