Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

impl Error trait for ParseWeekdayError and ParseMonthError #539

Merged
merged 1 commit into from
Jul 20, 2023
Merged

impl Error trait for ParseWeekdayError and ParseMonthError #539

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/month.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,16 @@ pub struct ParseMonthError {
pub(crate) _dummy: (),
Copy link
Collaborator

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?

Copy link
Contributor Author

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 since pub 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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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 😄.

}

#[cfg(feature = "std")]
#[cfg_attr(docsrs, doc(cfg(feature = "std")))]
impl std::error::Error for ParseMonthError {}

impl fmt::Display for ParseMonthError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "ParseMonthError {{ .. }}")
}
}

impl fmt::Debug for ParseMonthError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "ParseMonthError {{ .. }}")
Expand Down