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.

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

Support custom panic messages and add assert_eq! #224

Closed

Conversation

not-my-profile
Copy link

Resolves #223.

@not-my-profile not-my-profile changed the title Support custom panic messages and add assert_eq!. Support custom panic messages and add assert_eq! Aug 18, 2023

/// A [`std::assert_eq`] compatible wrapper around the [`Assert`] API.
#[macro_export]
macro_rules! assert_eq {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make sense to also add respective macros for assert_matches and assert_eq_path and assert_matches_path ... however I really dislike the names of the latter two, see #225. So I'd rather have us do a breaking rename before adding the new same-named macros.

@epage
Copy link
Contributor

epage commented Aug 18, 2023

  1. This is a breaking change and nothing in the PR description communicates that
  2. Let's first resolve whether something should happen in issues before moving on to PRs

@epage epage closed this Aug 18, 2023
@not-my-profile
Copy link
Author

The ! in the 2nd commit title indicates that the commit is breaking.

@epage
Copy link
Contributor

epage commented Aug 18, 2023

Yes, I had overlooked that. However, that should have a full "BREAKING CHANGES" section because the subject does not describe the breaking change.

@not-my-profile
Copy link
Author

Yeah that makes sense. I do have to note that I find your tendency to eagerly close PRs somewhat off-putting. You could just convert it to a draft.

@epage
Copy link
Contributor

epage commented Aug 18, 2023

I do have to note that I find your tendency to eagerly close PRs somewhat off-putting. You could just convert it to a draft.

If you are wanting to demonstrate something, you are free to create a branch and link to it or to create a draft yourself.

It is too easy for conversations to live too long in PRs that should instead be in Issues. There is also a social pressure to merge PRs, putting an undo burden on maintainers. Some maintainers take the approach of not caring about their users. Others try to bend over backwards for them and burn out. I'm trying to walk a middle ground. Pushing conversations to Issues by closing PRs is one of the boundaries I maintain.

@not-my-profile
Copy link
Author

Thanks for elaborating.

From your comment in #223:

Assert is a first class citizen in the API and should be able to be reused across calls (something which the current solution in #224 does not handle)

If you'd said that here, I'd have just converted the PR to a draft to signal that it's not ready and then either closed it or force pushed an alternative design that addresses your concerns.

I guess I have less of an issue with you closing PRs to move the discussion to issues and more of an issue with what you said before closing this issue:

  1. This is a breaking change and nothing in the PR description communicates that

Aside from the fact that PR descriptions can easily be updated, starting the first reply to a PR with "This is a breaking change" just does not feel very appreciative or welcoming.

@epage
Copy link
Contributor

epage commented Aug 21, 2023

Aside from the fact that PR descriptions can easily be updated, starting the first reply to a PR with "This is a breaking change" just does not feel very appreciative or welcoming.

Yes, maybe I could have better worded things.

I'd also recommend looking at this from a maintainer's perspective in the boldness of a PR being the first time the subject of a breaking change is broached combined with it never being explained and justified which should be front and center in the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom panic messages in snapbox
2 participants