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

Change how to format a data type with a record constructor #662

Merged
merged 9 commits into from
Jan 5, 2023

Conversation

toku-sa-n
Copy link
Collaborator

@toku-sa-n toku-sa-n commented Jan 5, 2023

After thinking a little, I've decided to specialize the case where a data or a newtype declaration has only one record constructor. It breaks the consistency, but I don't think people will define multiple record constructors often.

Fixes: #546

@mihaimaruseac By the way, could you merge PRs by squashing? It will make the commit log more readable. Of course, I can squash commits by myself, but it'll require force-pushing and I want to avoid it as I may make a mistake. Squashing-merging is easy and handy, I think.

@toku-sa-n toku-sa-n marked this pull request as ready for review January 5, 2023 07:09
@mihaimaruseac mihaimaruseac merged commit a7f3b2a into mihaimaruseac:master Jan 5, 2023
mihaimaruseac added a commit that referenced this pull request Jan 5, 2023
@mihaimaruseac
Copy link
Owner

Unfortunately, squashing has two effects which make it undesirable:

  • impossible to bisect over the commits on the branch since they're now all in one unit
  • reverting the PR results in an "unverified" commit

@toku-sa-n toku-sa-n deleted the data branch January 6, 2023 00:17
@toku-sa-n
Copy link
Collaborator Author

impossible to bisect over the commits on the branch since they're now all in one unit

I don't think this is a problem. We can still bisect to find a PR that introduced a bug and, if necessary, continue bisecting on the PR's branch to find the exact commit. It may be an extra, but tiny work. Rather, since all commits related to a PR are squashed, we can get a meaningful diff and easily access to the PR for the underlying discussion.

reverting the PR results in an "unverified" commit

I'm surprised by this behavior; I expected all commits created on GitHub would automatically be GPG-signed. Actually I don't care even if a commit is not signed, though.

Overall, I don't think squashing a PR has large problems. But if you are concerned about them, I don't have any objections to merge (not squash) PRs.

@mihaimaruseac
Copy link
Owner

reverting the PR results in an "unverified" commit

I'm surprised by this behavior; I expected all commits created on GitHub would automatically be GPG-signed. Actually I don't care even if a commit is not signed, though.

Yeah, I think it is a bug in GitHub here.

I am concerned about signed commits, is a step for supply-chain security.

But we also have very little need to revert PRs, so let's go with squashing.

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.

Formatting of newtype
2 participants