vaab / colour

Python color representations manipulation library (RGB, HSL, web, ...)
BSD 2-Clause "Simplified" License
319 stars 41 forks source link

Added a convenience function to create a random color. #54

Closed Helveg closed 4 years ago

codecov[bot] commented 4 years ago

Codecov Report

Merging #54 into master will decrease coverage by 2.85%. The diff coverage is 30.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
- Coverage   99.13%   96.28%   -2.86%     
==========================================
  Files           1        1              
  Lines         232      242      +10     
==========================================
+ Hits          230      233       +3     
- Misses          2        9       +7     
Impacted Files Coverage Δ
colour.py 96.28% <30.00%> (-2.86%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 11f138e...b80969a. Read the comment docs.

Helveg commented 4 years ago

For having such a small amount of files, the package structure is quite alien to me, where are the tests? 😄

vaab commented 4 years ago

Thanks for your interest in colour package. I read your code, what is your rationale for this addition ? While I can see the functionality, I'm afraid of a few points:

In general, maybe you could have answered to these questions already by writing and documenting the functionality itself in README.rst or in the doc of your functions/methods so as to convey to users and to reviewers your intention. This is of course missing (we need test and doc) and maybe stems from your ignorance of where are the tests... Well, there is 99% of coverage because every functionality is documented in the README.rst and also in docs provided along methods, or objects in colour.py, all this is automatically run by CI to insure us that every functionality is correctly presented in intention to users and that this doc is correctly tested. This is doctest.

To run all these tests you can use: nosetests -sx .

Helveg commented 4 years ago

The rationale is that it's more intuitive and convenient but I'll just keep this in my fork

vaab commented 4 years ago

I'm sorry if my answer seemed dry or offended you in any way. I hope I succeeded in sharing about how code inclusion can be added to the project (rationale in docs and tests). Any discussion is welcome here. I'm still curious and interested to hear more about the rationale (example of usage, and why you would need a random color without affecting it to at least an identifier), I feel these could be existing, but can't figure how exactly. While I can understand why you would qualify your proposal as "more intuitive and convenient", I'm not sure it can be alone answering the multiple points I have taken the time to pinpoint.

Helveg commented 4 years ago

Oh I closed it because I'm not experienced enough in either nosetests (I'm used to unit tests) or colors to adequately address your points and turn this into a qualitative pull request :) Thanks for taking your time to look into this.