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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the
_dummy
fields removed? This makes it possible for library users to construct the type, and we can no longer add fields in the future (not that I think we will...). Can you revert this part?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dummy field is not needed, constructing works by doing
let e = ParseMonthError;
, and why can't you add fields at a later time? I fail to see the issues you raised, sorry, especially sincepub struct ParseWeekdayError {}
is defined the same (which was not added by me).About running CI, not sure how to do that either, sorry. A push might work, but that implies a further needed commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A private field is a common future proofing trick. See the somewhat related https://rust-lang.github.io/api-guidelines/future-proofing.html#structs-have-private-fields-c-struct-private. But yes, it is not used consistently at the moment, and not all that interesting for this type.
But I wanted to call it out, as it is an unrelated change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see it as an unrelated change, as it makes it consistent with ParseWeekdayError. And even with your future proofing argument I still find this field unnecessary for this very application. However I can add it back, also to ParseWeekdayError if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you could please add it back and not change
ParseWeekdayError
than this PR will just be simple to approve 😄.