houseabsolute / Time-Local

Efficiently compute time from local and GMT time
https://metacpan.org/release/Time-Local/
Other
4 stars 11 forks source link

Confusing die message: "Day '30' out of range 1..28" from timegm() #20

Open sjvudp opened 10 months ago

sjvudp commented 10 months ago

Due to a programming error the month passed to timegm was one more than expected, so the 30th of January became the 30th of February, being invalid. However the error message as it is now is kind of confusing . Maybe it should convert the parameters to an ISO date string and saying something like "Day 30 in YYYY-02-30 is invalid; allowed range is 1..28". Due to leap years YYYY should be the actual year, preferably.

autarch commented 10 months ago

Yes, that makes sense. A PR to fix this would be great!

sjvudp commented 9 months ago

Yes, that makes sense. A PR to fix this would be great!

What's the difference between a "PR" and this github issue?

n1vux commented 9 months ago

PR = "Pull Request", which is GitHub's term for a user forwarding a "commit", (= a set of code changes in git) from their forked repo(sitory) to the upstream project repo.

(GitLab calls them MR="Merge Request".)

In the old days, voluntary open source projects said "Patches welcome" to indicate that if someone fixed a low urgency bug for themselves, to scratch the itch as we say, before the core team got around to it, the project would happily take the delta or patch and apply it upstream for everyone that much sooner.
His "would be great" is a modern and politer phrasing of the same sentiment.

sjvudp commented 9 months ago

PR = "Pull Request"

Ah, sorry, I thought "PR = Problem Report".

n1vux commented 9 months ago

(side note: such a PR would require modifying the self-test suite to pass with the new error(s), e.g. the following lines in file Time-Local/t/Local.t ;

                        $@, qr/Day '29' out of range 1\.\.28/,
                        $@, qr/Day '29' out of range 1\.\.28/,
                        $@, qr/.*out of range.*/,

and for consistency would do same for Month and likely do similar for Hour, Minute, Second (showing HH:MM:SS instead of YYYY-MM-DD) , so this is not a single trivial change, it's 2*(2+3) nearly trivial changes plus the testing & housekeeping involved with cutting a PR. 5 is the correct number since 'out of range' only appears in 5 croak calls; year has different messages - and much wider latitude in values.)

n1vux commented 9 months ago

Ah, sorry, I thought "PR = Problem Report".

Fair! In many ticketing systems it would be so. This GitHub one is optimized for being an add-on to git source-code version control, so it does differently; and GitLab does differently again. Since Feature Requests share space with Bugs/Problems here, but typically not with Help-Desk problems seen in standalone ticketing systes, there is a preference to have a generic neutral collective, hence ^Problems^ are called Issues here. And the offer of a contributed concrete solution is PR here, MR on GL. (And these aren't even a thing in a help-desk problem ticket system.) wiki

We think abbreviations facilitate communications, but the assumption of common set of abbrevs. is frequently wrong!