Improving code coverage for "base/printf.jl" #14410
Conversation
|
Looks like you need to fix whitespace: |
| elseif c=='s'; f = gen_s | ||
| elseif c=='p'; f = gen_p | ||
| else f = gen_d end | ||
|
|
There was a problem hiding this comment.
This line has trailing whitespace. Some programmers consider that bad, because it often litters the diffs with unrelated changes the next time someone edits this file.
As a result we added a check in the CI system so that tests fails when trailing whitespace is detected, so this needs to be fixed before the tests can execute successfully. @hayd has a great suggestion for how to let git automatically fix any such issues.
|
Thanks for the notes on whitespace! I obviously missed that part. |
| Int16(42), Int32(42), Int64(42), Int128(42), | ||
| #big"42" # causes stack overflow on %a ; gh #14409 | ||
| ) | ||
| @test( @eval(@sprintf($fmt, $num) == $val)) |
There was a problem hiding this comment.
Despite the original test file, the conventional way to write this is @test @eval(@sprintf($fmt, $num) == $val). It might also make sense to reverse the @eval and the @test: @eval @test @sprintf($fmt, $num) == $val.
There was a problem hiding this comment.
Hmmm, I personally like the last one best because it places only the relevant code part inside the test macro (instead of placing the eval in there as well). But on the other hand the first is more obviously a test, and technically more correct (as it includes the eval in the test macro). I'll go with convention then.
There was a problem hiding this comment.
The convention part is just the lack of parens with spaces inside of them. I don't think we have a convention around whether @test or @eval comes first but I agree that having the @eval outside seems (marginally) better.
mason-bially
force-pushed
the
printf-cov
branch
2 times, most recently
from
db7f4d2 to
c38a00b
Compare
* Refactoring branch in "base/printf.jl" for correct code coverage results. * Changing an exception type in "base/printf.jl" to be more specific.
mason-bially
force-pushed
the
printf-cov
branch
from
c38a00b to
dce82c5
Compare
|
These are now included in #15377. Thanks for starting these. |
|
I'm glad someone managed to get these changes working. That being said @thepulkitagarwal striped my authorship from the change, which would also prevent me from being listed in the copyright holders list used in the julia LICENSE file. Both of which are violations of the MIT license under which I originally contributed these changes. My changes account for more than 90% of the complete pull request, specifically commit 8bc9e23. Please fix your license violation (by fixing the authorship, rewriting the code from scratch, or not using it). |
|
Your contribution has been recorded: 5baedf4. |
|
Misspelling my name and pushing straight to master, very mature. That only really fixes one of the two problems... I had meant for @thepulkitagarwal to correctly attribute authorship of 8bc9e23 my comment was directed at him. I really don't want to deal with this community anymore: I give up. |
|
Your commit is now in master: dce82c5 https://github.com/JuliaLang/julia/commits/master?author=mason-bially (I think that Stefan's above commit glued that up for you). I agree your commit should have been appended to, rather than nuked/reset. Unfortunately mistakes do happen, especially with git... and especially on an open source project. I'm sure misspelling your name in the gluing commit message was a mistake rather than a slight. |
|
Misspelling your name was unintentional – sorry about that. Since there were no file changes, pushing to master wasn't exactly risky. (Last I tried it, PRs with no file changes also kind of confuse GitHub.) @thepulkitagarwal did reference your PR from his commit message, so it's not as if he tried to pass off your work as his own, but yes, appending to your branch would have been better. Since git history is immutable, there is nothing to be done about it now aside from what I did: record your commit too. Since your original commit is now in the project history, you have gotten full credit for your contribution. I hope that you find a project that you enjoy contributing to. Best of luck! |
Which is why I said exactly:
It was risky enough for me to now have my own personal simple demonstration of the dysfunction of the Julia community. It went from a "I don't really care what happens" to a "you have got to be kidding me", you moved me from neutral on the Julia community to negative on the Julia community. Maybe don't straight push to master in the future, especially when the correct (better, safer) solution was rebasing a commit on a branch. Keep the luck, it seems terrible. |
|
@mason-bially: I did clone your branch on my computer, and tried to merge it with the current master, but unfortunately I couldn't get it to work. I couldn't figure out what was wrong in your code and so, I restarted the entire thing, and incrementally changed the code to remove/change the parts that weren't working - which was easier for me. I apologize for that. Also, this was a one-off mistake by one person (me), and not by the organization at large. I hope that you will reconsider your decision about leaving this community. |
|
@thepulkitagarwal it has very little to do with you. It's systemic with the larger community and it's leadership. It isn't your fault. I already left before I could get in deep enough. This was just my lingering contribution. I actually had a working version that I was just waiting on another branch to merge, I honestly don't care though. |
|
It's a little unclear from github, but I believe Stefan's merge commit was also reverting your change so the net effect of the two commits is zero code change to master. You could push empty commits all day and not really risk anything other than modifying the history (which was Stefan's intent in this case) and consuming CI time. @mason-bially there's no malice intended here at all despite the impression you seem to be under, but I do think it would be best for everyone involved if we went our separate ways and (voluntarily) ceased all further interactions. |
After that business with Dan Luu, banning of the person who helped me, and now this? This is strike three for me. But yea, this will be my last post. |
|
Good luck! |
See also #14409