refactor distinct #95
Conversation
|
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 ( |
|
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, ***@***.***> 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
<#95?email_source=notifications&email_token=AAATSBGKRXRWUMVLTMWL62DQKXRDDA5CNFSM4IXKNDF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7IOBHY#issuecomment-533782687>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAATSBB45JSQNQSP4XTEPILQKXRDDANCNFSM4IXKNDFQ>
.
|
rivy
force-pushed
the
rf.distinct
branch
3 times, most recently
from
b273b5a to
0cc918e
Compare
|
Forgot to re-enable the Travis deployment section ... that's fixed. Everything is packaged in separate commits to drop CI, renaming, etc. as you wish. |
|
@sharkdp Do you have any remaining issues with this PR? I'd be happy to address them. |
|
This is the minimal change, everything else stripped out. |
src/cli/commands/distinct.rs
Outdated
| Box::new(|stats: &IterationStatistics| { | ||
| let stderr = io::stderr(); | ||
| let brush_stderr = Brush::from_environment(Stream::Stderr); |
There was a problem hiding this comment.
why have these lines been moved into the closure? This way we lock stderr and create a new brush for every callback iteration.
There was a problem hiding this comment.
I'm somewhat new to rust and I think that's a good question about what may be an incorrect solution on my part, but where would you put the lock for the output?
I though that here, nearest the output was the most correct. I may be incorrect in my understanding but I imagine handing the lock to the callback from elsewhere would lead to deadlocks.
I'd love to be shown a better, more idiomatic way...
There was a problem hiding this comment.
We are not running the callback concurrently. If we were, I guess we would actually introduce a potential synchronization overhead if we lock stderr on each iteration. But there is no deadlock scenario here. I modified your PR slightly to lock stderr only once (I didn't really look closely. My original code actually locked stderr multiple times as well, so sorry for that).
|
Thank you very much. The changes look great. I only have a minor comment. |
|
Looks great, I'm looking forward to using this! |
|
👍 |
distinct_colors()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_colorsas the name when I was experimenting and left that in as a single commit ... easy to remove if you preferfixed_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.