search

JuliaIO / LibExpat.jl

Julia interface to the Expat XML parser library
Other
9 stars 32 forks source link

float and int parsing and conversion for 5.0 #64

Closed beOn closed 7 years ago

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 28.657% when pulling d913840cda80d2662e0c93f507e8d7cb66537463 on beOn:pull-request/d913840c into 8c82be3ab1e790886e3eacbba4430b0af275b2cd on amitmurthy:master.

musm commented 7 years ago

If you would like a couple of tests would be very welcome to make sure we catch these out

tkelman commented 7 years ago

@musm don't do "Rebase and merge," github does not include the reference to the PR # in the commit message when you do that.

And you should generally leave PR's open for longer on packages that you're not the sole author of. I've warned you about this before.

johnrichardrinehart commented 7 years ago

@tkelman, forgive me for intruding. I don't contribute to this package. But, your last comment is a bit strange. If a collaborator (which implies status) wants to merge in a pull request then they shouldn't have to wait for some arbitrary length of time.

I think it's important to remember that Julia exists in a non-profit context and we're all contributing our time because we think our work is valuable. No one out here is rushing to merge PRs to meet deadlines (because we don't have them). If a PR is merged in, then it must be that the work was thought to be beneficial to the project.

Also, with the power of git it's easy to revert small PRs if we really have to.

tkelman commented 7 years ago

@musm has not had contributor access here for long at all. Code review matters, particularly people who are depending on this package.

johnrichardrinehart commented 7 years ago

Right, but because he (gender assumption) is a contributor it means that he's trusted. If I'm not mistaken, you're also not the project maintainer. Also, I see that the merge failed to pass CI testing, but that doesn't imply there wasn't code review. The tests may need to be changed to reflect that the package is working. In that case, the changes to the useable part of the code base would be fine.

I respect your ideas, I just think that this particular commit may have been a weak place to voice them. All that happened was the introduction of parse and int -> Int. If people who rely on this package have problems then they can raise an issue :). We have them; we might as well use them.

How long would you want someone to wait before merging in a PR as a contributor?

musm commented 7 years ago

In this commit, parsefloat was deprecated in 0.5 and thus these methods no longer work on 0.5 (it wasn't being tested for). Similar with the other changes.

The nightly failures are unrelated and due to a new world error related to callbacks. The tests pass on 0.4 and 0.5.

The PR is uncontroversial bug fix (thank you @beOn).

beOn commented 7 years ago

Sure thing :) On Wed, Jan 11, 2017 at 2:44 PM Mus M notifications@github.com wrote:

In this commit, parsefloat was deprecated in 0.5 and thus these methods no longer work on 0.5 (it wasn't being tested for). Similar with the other changes.

The nightly failures are unrelated and due to a new world error related to callbacks. The tests pass on 0.4 and 0.5.

The PR is uncontroversial bug fix (thank you @beOn https://github.com/beOn).

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/amitmurthy/LibExpat.jl/pull/64#issuecomment-271988506, or mute the thread https://github.com/notifications/unsubscribe-auth/AAC-RIqROK7DdgY_B8f_cA0nxL1NRt2Zks5rRT81gaJpZM4LgpML .

tkelman commented 7 years ago

People who contribute to Julia and its packages are based all around the world. Given time zones, waiting at least a day for anything non-urgent to allow people to see, be aware of, and comment on a PR would be sufficient and recommended.

Given the lack of test coverage hitting these lines, it would have been very useful to add a test here.

beOn commented 7 years ago

Alas. On Wed, Jan 11, 2017 at 3:00 PM Tony Kelman notifications@github.com wrote:

People who contribute to Julia and its packages are based all around the world. Given time zones, waiting at least a day for anything non-urgent to allow people to see, be aware of, and comment on a PR would be sufficient and recommended.

Given the lack of test coverage hitting these lines, it would have been very useful to add a test here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/amitmurthy/LibExpat.jl/pull/64#issuecomment-271992831, or mute the thread https://github.com/notifications/unsubscribe-auth/AAC-RKX6qXEKnZqui2xioTdaII6T3ZBSks5rRULwgaJpZM4LgpML .