Add nanoseconds without dots

[emschwartz]

Resolves #250

[quodlibetor]

Looking great!

Regarding your questions:

• Yeah, I think we should duplicate the nanosecond parse tests, all of them. You can verify that you are only finding nanoseconds without dots by doing something like:

  check!(".400", [fix!(Nanosecond3NoDot)]; INVALID);  // probably not actually INVALID
  check!("400", [fix!(Nanosecond3NoDot)]; nanosecond: 400_000_000);

  Does that make sense?

• Only other place to update is the strftime docs and the CHANGELOG.md

[quodlibetor]

I took a little time to look at it, and modifying the beginning of fn.parse to include a print:

    for item in items {
        println!("s: {:?} item: {:?} parsed: {:?} ", s, item, parsed);
        match item {

results in this error:

s: "421950803547" item: Fixed(Nanosecond9NoDot)
parsed: Parsed { year: None, year_div_100: None, year_mod_100: None, isoyear: None, isoyear_div_100: None, isoyear_mod_100: None, month: None, week_from_sun: None, week_from_mon: None, isoweek: None, weekday: None, ordinal: None, day: None, hour_div_12: None, hour_mod_12: None, minute: None, second: None, nanosecond: None, timestamp: None, offset: None, _dummy: () }

s: "" item: Numeric(Second, None)
parsed: Parsed { year: None, year_div_100: None, year_mod_100: None, isoyear: None, isoyear_div_100: None, isoyear_mod_100: None, month: None, week_from_sun: None, week_from_mon: None, isoweek: None, weekday: None, ordinal: None, day: None, hour_div_12: None, hour_mod_12: None, minute: None, second: None, nanosecond: Some(421950803), timestamp: None, offset: None, _dummy: () }

thread 'format::parse::test_parse' panicked at 'assertion failed: `(left == right)`
  left: `Err(ParseError(TooShort))`,

which strongly suggests that Nanosecond9NoDot is not correctly stopping parsing after the first 9 digits (since the nanosecond field is correct), but that the surrounding parse code is still consuming the entire str.

[emschwartz]

Good catch. It was an issue with using the try_consume macro with a slice of the string. try_consume updates s in place, but if you pass a slice it forgets about the whole rest of the string

[quodlibetor]

did you want to uncomment this?

[emschwartz]

Yeah, I'm fixing the test. I'll add another commit to the PR tomorrow

[quodlibetor]

Still looking good, there are two new changes:

• You're going to need to either rebase or merge in master, and you'll probably get conflicts in the changelog
• Could you move your new variants into the InternalInternal struct, the equivalent of this commit for your changes? It didn't exist when you started work, but I think that, sadly, it's necessary for strict semver compat.

[emschwartz]

Yeah, I'll make those changes tomorrow.

Would you prefer me to squash all the commits into one or leave them as they are?

[quodlibetor]

The commits are nicely separated as is, no need to squash. I would also appreciate the refactoring to use InternalInternal to be its own commit, I hope to revert it with the next breaking change.

Speaking of it, though, if you are going to be editing your test commit and you use git from the command line I have a very new project that I'm looking for beta testers/bug reporters/contributors on that is supposed to make editing old commits a bit nicer.

[emschwartz]

Could you move your new variants into the InternalInternal struct, the equivalent of this commit for your changes? It didn't exist when you started work, but I think that, sadly, it's necessary for strict semver compat.

I'm not sure I understand that. Is the idea to make it so this feature can't be accessed by the user? What is the semver issue (it's a non-breaking change)?

[quodlibetor]

Adding new variants to a public enum is a major breaking change. The idea isn't to prevent usage of the feature, it's to require that the only usage of the feature is through the parse method, specifically so that we don't break semver.

The hack used by the ecosystem to mark enums as open is to have a #[doc(hidden)] __DoNotMatchAgainstMe variant and to include in the docs for an enum that additions to it are expected. However, chrono was created before that convention existed, and so it created the separation of internal and external enums. The internal enum is not public, and so it can be expanded without trouble. The external enum, though, is public and so technically someone could be doing an exhaustive match against the whole thing.

For most libraries I think that just having bad docs would mean that you can just fix them and improve life, but chrono is one of the most depended-on crates in the ecosystem and so if any crate is going to have someone doing crazy things against massive enums then chrono might be it.

[quodlibetor]

specifically, the only things that you should need to change should be to move your variants to the InternalInternal enum and to change your uses of fix! to internal_fix!, all tests (in particular tests that verify parse) should continue to work without any other changes.

[emschwartz]

Gotcha. Thanks for the explanation. Should it be InternalInternal or InternalFixed?

(Never mind, think I got it)

[emschwartz]

Made the change. Ready for re-review

[quodlibetor]

LGTM, I'm going to do one more review pass this evening, I don't recall any other possible concerns so hopefully I'll get this published tonight.

[emschwartz]

Sounds good. Thanks for the help!