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

Fix #14341, rewrite bytes2hex to be more efficient #14397

Closed
wants to merge 2 commits into from

Conversation

ScottPJones
Copy link
Contributor

Add hex(x::Vector{UInt8}) & hex(x::ByteString)

Add hex(x::Vector{UInt8}) & hex(x::ByteString)
@ScottPJones
Copy link
Contributor Author

Using the (unregistered) Benchmarks.jl package, this looked to be around 25x faster on the simple tests I tried.

@@ -234,4 +234,19 @@ function hex2bytes(s::AbstractString)
return a
end

bytes2hex(arr::Vector{UInt8}) = join([hex(i,2) for i in arr])
function hex(arr::Vector{UInt8})
out = Vector{UInt8}(sizeof(arr)<<1)
Copy link
Contributor

Choose a reason for hiding this comment

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

why sizeof, isn't length what you care about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a vector of bytes, sizeof and length are the same, but I can change it

@ScottPJones
Copy link
Contributor Author

@tkelman Does this look OK to you now? Thanks!

@StefanKarpinski
Copy link
Sponsor Member

No one could provide a coherent definition of hex that includes this behavior, so we're not going to do this. If you want to make the improved version of bytes2hex without this part, we could merge that.

@ScottPJones
Copy link
Contributor Author

That includes what behavior? I think it is pretty clear what hex(data::Vector{UInt8}) should be
(and my colleagues have stumbled across that before, expecting it to work).
I think the bytes2hex (and num2hex) names should just be deprecated (in another PR).

@StefanKarpinski
Copy link
Sponsor Member

That was not the consensus of the issue #14341 thread, which you seem to have entirely ignored.

@ScottPJones
Copy link
Contributor Author

No, I hadn't ignored it at all, I just hadn't had time to respond yet (it was the weekend),
and I don't think you are correct about the consensus there.
@JeffBezanson Could you take a quick look at this, and possibly reopen it? Thanks.

@StefanKarpinski
Copy link
Sponsor Member

If you separate out the change to bytes2hex without the part that makes bytes2hex a method of hex, then that part can be merged.

@ScottPJones
Copy link
Contributor Author

  1. I'm not able to update it, because you have closed it.
  2. As far as the "consensus" in rewrite bytes2hex to be more efficient #14341, you were the only person who objected to replacing bytes2hex with a method of hex, @nalimilan outright said that it should be hex, and the gist of @mason-bially comments also seem to agree, and @tkelman's comment "function names with 2 in them do often seem like artifacts of languages that avoid dealing with types" also seems to be on the side of hex instead of bytes2hex. That's at 3 or 4 to 1.

@nalimilan nalimilan reopened this Dec 15, 2015
@StefanKarpinski
Copy link
Sponsor Member

Even when this project was just four people, we didn't just make changes when one person out of the four still had unaddressed objections. There's an obvious uncontroversial change here which is specifically what issue #14341 is about. (An issue, incidentally, which I marked as "intro" so that someone new to the project would have the chance to do something small, simple and self-contained). The original issue does not mention merging bytes2hex into hex – that's something you decided the issue was about. Instead of making the completely uncontroversial change that would fix the original issue, you mixed that change in with another change that did not have consensus, but which you wanted made.

If this was occurring in isolation it would be no big deal. But it isn't. You've done this over and over again. Almost every PR you've made mixes far too many changes together, many of which are clearly still controversial, but which you’ve decided you want made. When people point this out, you make a big fuss about it (like you are here), argue with them (like you are here), and much of the time, you end up winning by attrition: people get so sick of dealing with you arguing that after a while they just give up and leave the discussion, letting others carry on, until the last person standing finally merges your PR even with contentious changes still in it, just so the annoyance will stop. By this particularly banal and obnoxious form of bullying, you've ended up getting your way repeatedly. You've succeeded in bulldozing many changes that you wanted through, despite lack of consensus, frankly, just because everyone involved was tired of dealing with you.

The Julia community has now been dealing with this for almost a year. This is a warning. If you do this again, I will block you from the JuliaLang repo and that will be that.

@ScottPJones
Copy link
Contributor Author

See ScottPJones@6d11a70.
I had already removed the two lines, when you closed this (I was simply doing due diligence and running a full test suite before pushing the change)

  1. I added did that name change because there was already a consensus that bytes2hex wasn't really a good name for the functionality, before you started started complaining about it.
  2. After @nalimilan also said it should be just hex, I went ahead and did so, it wasn't just my idea.
  3. I don't think that the changes of mine that have been merged in over the last 7 months (almost a year? I just started at the end of April!) have been so controversial as you paint, and if they were so bad as you have stated, I'm sure that you or others would have reverted them. So far, the only thing that has been changed afterwards was not using milliseconds, nanoseconds, etc. for the @time macro (note: @benchmark also uses ms, ns, etc. (with the Unicode character for microsecond)). (I do think the final form is better now for eyeballing differences, so I'm happy about it being changed from my first attempt).

@JuliaLang JuliaLang locked and limited conversation to collaborators Dec 15, 2015
@StefanKarpinski
Copy link
Sponsor Member

For those reading this, @ScottPJones has been blocked. To provide some context and background, his behavior has been an ongoing community issue for a long time now – the exact time doesn't really matter. What does matter is that it has been brought up many times by a variety of people, and on each occasion the complaint has been met with further argument and no lasting improvement in disruptive behavior. This warning has been met yet again with argument rather than any sort of acknowledgement of the problem or willingness to change. Since this essentially eliminates any hope for cessation of problematic conduct, blocking is the only viable option remaining to protect the community and project.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants