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

Prefix type parameters and lifetimes with double underscore #315

Merged
merged 1 commit into from
May 11, 2016
Merged

Prefix type parameters and lifetimes with double underscore #315

merged 1 commit into from
May 11, 2016

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented May 9, 2016

Addresses #312. We already do this almost everywhere.

@dtolnay
Copy link
Member Author

dtolnay commented May 9, 2016

The Travis failure is due to #313.

@oli-obk
Copy link
Member

oli-obk commented May 10, 2016

up to now the "style" of PRs was to rebase them instead of merging them. I think we should stay with that style.

@dtolnay
Copy link
Member Author

dtolnay commented May 10, 2016

Okay I rebased this PR and will rebase my other ones.

We can discuss this in a separate ticket if you are interested, but just to raise some points in favor of merge commits:

  1. A large part of the point of PRs is code review. As PRs change, GitHub keeps track of what part each reviewer has seen. Every rebase screws that up and forces reviewers to start from the beginning.
  2. Many times a PR has already been reviewed fully but is not ready to merge for a variety of reasons. If there are only merge commits since the time that it was reviewed, it is probably okay to merge. On the other hand if there is a single rebased commit, you have no idea. Better review the whole thing again to make sure nothing important has changed.
  3. The "Files changed" view is no different whether there are merge commits or not. It shows you the diff compared to the base branch. So if that is your workflow, it is not hindered by merge commits.
  4. If we care about preserving a relatively simple commit history, PRs that include merge commits can be rebased and squashed right at the end, after the review is finished.

@oli-obk
Copy link
Member

oli-obk commented May 10, 2016

I find your arguments very convincing, but I don't want to change the style without consulting @erickt, so lets open a new issue for now

@oli-obk oli-obk merged commit 6596f77 into serde-rs:master May 11, 2016
@dtolnay dtolnay deleted the underscore branch May 11, 2016 16:16
@dtolnay dtolnay added this to the v0.7.5 milestone Jun 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants