raimon49 / pip-licenses

Dump the license list of packages installed with pip.
MIT License
307 stars 43 forks source link

refactor: parameter license_classifier expects a list #133

Closed b-kamphorst closed 1 year ago

b-kamphorst commented 1 year ago

Alternative solution to https://github.com/raimon49/pip-licenses/pull/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 commented 1 year ago

I actually intended to write a test that captures the issue that https://github.com/raimon49/pip-licenses/pull/132#issue-1427355947 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?

codecov[bot] commented 1 year ago

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.

raimon49 commented 1 year ago

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,

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