search

sharkdp / pastel

A command-line tool to generate, analyze, convert and manipulate colors
Apache License 2.0
5.08k stars 102 forks source link

refactor distinct #95

Closed rivy closed 5 years ago

rivy commented 5 years ago

During testing, I ran into 32/64-bit differences in outcome and modified the tests to compensate.

I re-implemented the changes based on your fixed color implementation. (I had been using invariant_colors as the name when I was experimenting and left that in as a single commit ... easy to remove if you prefer fixed_colors).

To facilitate testing, I also went down the AppVeyor CI rabbit-hole for a couple of days and emerged with a generally flexible and functional configuration (including 32-bit and 64-bit, as well as, MSVC and GNU compilation).

I modified the Travis CI to include 32-bit compilation. I also saw that Windows compilation was now being included, but I marked them as possible failure and left the AppVeyor configuration commit since the Travis implementation is in "preview".

I also ended up adding an EditorConfig file as a commit, just to stop my IDE from grumbling.

Let me know what you think. I'm happy to modify or drop any of the commits.

sharkdp commented 5 years ago

Thank you for your contribution.

I'm sorry, but this PR is too much. There are so many unrelated things going on and there are definitely some things which I do not want to include (.editorconfig) and some modifications which I would not accept (the deployment part in .travis.yml is completely disabled).

rivy commented 5 years ago

They're all in seperate commits and easy to drop. Just let me know what you want / don't.

On Sat, 21 Sep 2019, 04:17 David Peter, notifications@github.com wrote:

Thank you for your contribution.

I'm sorry, but this PR is too much. There are so many unrelated things going on and there are definitely some things which I do not want to include (.editorconfig) and some modifications which I would not accept (the deployment part in .travis.yml is completely disabled).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sharkdp/pastel/pull/95?email_source=notifications&email_token=AAATSBGKRXRWUMVLTMWL62DQKXRDDA5CNFSM4IXKNDF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7IOBHY#issuecomment-533782687, or mute the thread https://github.com/notifications/unsubscribe-auth/AAATSBB45JSQNQSP4XTEPILQKXRDDANCNFSM4IXKNDFQ .

rivy commented 5 years ago

Forgot to re-enable the Travis deployment section ... that's fixed. Dropped EditorConfig commit.

Everything is packaged in separate commits to drop CI, renaming, etc. as you wish.

joshhansen commented 5 years ago

@sharkdp Do you have any remaining issues with this PR? I'd be happy to address them.

rivy commented 5 years ago

This is the minimal change, everything else stripped out.

sharkdp commented 5 years ago

Thank you very much. The changes look great. I only have a minor comment.

joshhansen commented 5 years ago

Looks great, I'm looking forward to using this!

rivy commented 5 years ago

👍