Return NA for malformed period strings; fix P2H30M meaning months - #1231
Open
billdenney wants to merge 4 commits into
Open
billdenney wants to merge 4 commits into
billdenney wants to merge 4 commits into
Conversation
The period parser accepted several malformed inputs and returned a value
nobody asked for: ".h" gave 0, "PTM" gave 1 minute, "P1D1D" gave 2 days,
"T1H" gave 1 hour, "P1DT" gave 1 day, and in "1h (2h (3h) 4h) 5h" the first
")" ended the skipped group so the result was 10 hours.
These now return NA, as "1X" already does:
- a "." with no digit on either side is not a number;
- after an ISO "P", a single-letter designator needs a number and may occur
only once;
- "T" needs a "P" or a component before it, a component after it, and occurs
once;
- "(" cannot open a second group inside a skipped "(...)". The skip also
stops at the end of the string.
Unit names without a number ("day day"), repeated shorthand units, "T"
after a component without "P" ("10DT10M", which the docs use), and mixed
ISO and shorthand strings parse as before.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Regenerated with devtools::document() on an unmodified tree. roxygen2 8.1.0 replaces RoxygenNote with Config/roxygen2/version, regroups the NAMESPACE imports and rewrites \linkS4class links. No function or documentation source changed, so this commit can be skipped in review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
"2008-05-11/P2H30M" and "08 05 11/P 2h 30m" both ended on 2010-11-11, because "M" after an ISO "P" and lowercase "m" in shorthand mean months. The neighbouring example "P2hours 30minutes" shows the intent. Use "PT2H30M" and "30min", and pin all three in a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
"P2H30M" is used in lubridate's own interval tests and, until the previous commit, its documentation, plainly meaning 2 hours 30 minutes. Because "M" after "P" always meant months, it parsed as 2 hours and 30 months. An hours or seconds designator before "T" now starts the time part, so a following "M" is minutes. "M" before any time designator is still months. This changes the value of an input that used to parse, so it is kept as a separate commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Behaviour change. These malformed strings returned a silent value; they now return
NA, as"1X"already does:".h""PTM""P1D1D""T1H""P1DT""1h (2h (3h) 4h) 5h"The rules: a
.needs a digit on one side. After an ISOP, a single-letter designator needs a number and may occur once.Tneeds aPor a component before it, and a component after it.(cannot open a second group inside a skipped(...).Unaffected: unit names without numbers (
"day day"), repeated shorthand units,"10DT10M"(documented), mixed ISO and shorthand strings,format()round-trips, and"P1H"withoutT, which the interval docs use.The second commit fixes two
interval()examples that spanned 2.5 years instead of the 2.5 hours they intend."2008-05-11/P2H30M"and"08 05 11/P 2h 30m"both ended on 2010-11-11. They now use"PT2H30M"and"30min", and a test pins them.The last commit changes the meaning of an input lubridate's own documentation and tests used, so it is separate and can be dropped. In an ISO period without
T, anHorSdesignator now starts the time part, so a followingMmeans minutes:Mbefore any time designator is still months, so"P3M2H"is 3 months 2 hours.The branch also carries the roxygen2 8.1.0 regeneration commit from the signed-durations PR, unchanged. It is needed for the interval.Rd update.
🤖 Generated with Claude Code