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

Remove requests or suggestions about rebase and fixup contradictory to rust-highfive bot comment #1111

Merged
merged 3 commits into from
Jun 24, 2021
Merged

Remove requests or suggestions about rebase and fixup contradictory to rust-highfive bot comment #1111

merged 3 commits into from
Jun 24, 2021

Conversation

jhg
Copy link
Contributor

@jhg jhg commented Apr 18, 2021 •
edited
Loading

Closes #900 (see the issue's comments for more details)

jhg added 2 commits April 18, 2021 19:52
@jhg
Remove request of rebase to fixup commits to follow recommendation of…
ff66b08 
… rust-highfive bot

The bot comment: "If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes."
@jhg
Remove suggestions about rebase to fixup or add additional changes re…
2b47fd3 
…quested

To avoid confusion with the recommendation of rust-highfive bot
@jhg jhg changed the title Remove requests or suggestions about squashing-vs-pushing Remove requests or suggestions about rebase and fixup contradictory to rust-highfive bot comment Apr 18, 2021
jyn514
jyn514 reviewed Apr 18, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Could you also make a PR to https://github.com/rust-lang/highfive removing the comment to add new commits as follow-ups?

src/git.md Outdated Show resolved Hide resolved
src/git.md Outdated Show resolved Hide resolved
@jhg
Copy link
Contributor Author

jhg commented Apr 18, 2021

Could you also make a PR to https://github.com/rust-lang/highfive removing the comment to add new commits as follow-ups?

Yes, I'll check it. 😄

@jhg
Copy link
Contributor Author

jhg commented Apr 18, 2021 •
edited
Loading

@jyn514 I found the texts of highfive bot and I have 2 possible changes:

- If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

To:

+ If any changes to this PR are deemed necessary, please ask to the reviewer their preference about how to add them. This should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Or:

+ If any changes to this PR are deemed necessary, tell the reviewer about it. This should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

The first advise about it's better to ask to the reviewer (specially if one is new could avoid frustration to do something again in other way) and the second only remove the part about how to push changes.

Which could be better? 🤔

@jyn514
Copy link
Member

jyn514 commented Apr 18, 2021

To be honest, I think all that is overkill. If you don't know whether to squash or add new commits, either is fine, no one will get mad if you pick the other option. If the reviewer cares they'll mention something before merging. I'd suggest just removing that paragraph altogether.

@jhg
Copy link
Contributor Author

jhg commented Apr 18, 2021 •
edited
Loading

I also was thinking that but I was not sure if the end of that could be useful for someone. I agree to remove the paragraph. 😃

@jhg jhg mentioned this pull request Apr 18, 2021
Merged
@jhg
Copy link
Contributor Author

jhg commented Apr 18, 2021

@jyn514 the PR in highfive bot is rust-lang/highfive#327 😄

@jhg
Copy link
Contributor Author

jhg commented Apr 18, 2021

Also I pushed a new commit reverting the changes in src/git.md.

@jyn514
Copy link
Member

jyn514 commented Apr 26, 2021

Just FYI - this looks good to me as-is, but I'd prefer to wait to merge until rust-lang/highfive#327 gets merged so people don't think "add more commits" is the accepted advice.

@jyn514 jyn514 added the S-blocked Status: this PR is blocked waiting for something label Apr 27, 2021
@JohnTitor JohnTitor added S-waiting-on-review Status: this PR is waiting for a reviewer to verify its content and removed S-blocked Status: this PR is blocked waiting for something labels Jun 24, 2021
JohnTitor
JohnTitor approved these changes Jun 24, 2021
View reviewed changes
Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

We can now merge this as the PR on highfive has been merged. Thanks!

@JohnTitor JohnTitor merged commit 48d01f8 into rust-lang:master Jun 24, 2021
@ehuss ehuss mentioned this pull request Jul 7, 2021
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 7, 2021
@JohnTitor
Rollup merge of rust-lang#86919 - ehuss:update-books, r=ehuss
5196885 
Update books

## nomicon

8 commits in b9ca313e687c991223e23e5520529815dc281205..7a13537f96af4b9b8e3ea296d6e5c3c7ab72ce9f
2021-06-22 12:02:20 -0400 to 2021-07-05 23:34:47 -0400
- Apply review comments
- Fix some style issues
- Move the list of coercions to the reference
- Add an example that shows the null-pointer opt does not happen
- Remove casting list from the nomicon (rust-lang/nomicon#287)
- Audit `ignore` annotations (rust-lang/nomicon#288)
- rename typo "lifetime" to "reference" (rust-lang/nomicon#286)
- Add an incomplete warning to the top page (rust-lang/nomicon#274)

## reference

7 commits in d9699fa8f3186440fdaadd703d63d8d42322c176..ab60513a3a5a0591e237fddff5d027a982648392
2021-06-21 12:23:10 -0700 to 2021-07-05 08:27:31 -0700
- fix grammar in Expressions (rust-lang/reference#1057)
- fix comment in function parameter drop scope example (rust-lang/reference#1056)
- fix typo in macro-ambiguity.md (rust-lang/reference#1058)
- Mention (negative) infinity values on float-to-int casting (rust-lang/reference#1054)
-  (rust-lang/reference#841)
- Missing TypeParamBounds in TypeAlias (rust-lang/reference#1036)
- Be more precise about array offset in type layouts (rust-lang/reference#1034)

## book

34 commits in 55a26488ddefc8433e73a2e8352d70f7a5c7fc2b..a90f07f1e9a7fc75dc9105a6c6f16d5c13edceb0
2021-05-09 12:03:18 -0500 to 2021-07-05 14:43:12 -0400
- Clarify ?Sized syntax. Fixes rust-lang/book#2422.
- Add some notes that macros are different than functions
- Break up a long sentence. Fixes rust-lang/book#2329.
- Further clarify and make consistent the reference to deref coercion
- Update ch04-03-slices.md
- add usage for `String` reference
- Update ch15-02-deref.md (rust-lang/book#2780)
- Remove claim about performance of i32
- Reword to avoid awkward pluralization
- Make the link to the reference relative
- Merge remote-tracking branch 'origin/pr/2753'
- Reword number of library crates a package contains (rust-lang/book#2750)
- Clarify explanation of why you can test private functions; add link
- Merge remote-tracking branch 'origin/pr/2743'
- Fix code hiding that I broke in eb60fedc9
- Link to the exact later section we're talking about
- improve cross-references for newtype pattern
- ch12-05, listing 12-20: Add missing "does not compile" warning (rust-lang/book#2731)
- cargo format
- Merge remote-tracking branch 'origin/pr/2724'
- Remove ordinal numbers and only refer to indexes to avoid confusion
- Let's mention the former and current authors of tlborm.
- Update tlborm link to point to Veykril's up-to-date version (rust-lang/book#2722)
- Merge remote-tracking branch 'origin/pr/2720'
- Describe the ferris pictures in the alt text
- Merge remote-tracking branch 'origin/pr/2707'
- Reword ... explanation to include the word deprecated, list that first
- Precise that the `...` inclusive range pattern has been replaces (rust-lang/book#2714)
-  (rust-lang/book#2696)
- fix typo: missing "type" after generic (rust-lang/book#2777)
-  (rust-lang/book#2709)
- Remove sentence about how Rust used to be
- Fix a potentially confusing statement about static lifetimes of static variables. (rust-lang/book#2692)
- Replace 'which'. (rust-lang/book#2663)

## rust-by-example

2 commits in 805e016c5792ad2adabb66e348233067d5ea9f10..028f93a61500fe8f746ee7cc6b204ea6c9f42935
2021-05-20 17:08:34 -0300 to 2021-07-06 06:28:53 -0300
- Fix a couple of typos in the `integration_testing.md` file (rust-lang/rust-by-example#1448)
- Fix Structures type list (rust-lang/rust-by-example#1446)

## rustc-dev-guide

13 commits in fe34beddb41dea5cb891032512a8d5b842b99696..60e282559104035985331645907c3d9f842312c5
2021-06-21 21:50:12 +0200 to 2021-07-05 11:21:03 -0400
- Fixed typos in inline code
- Document lang items (rust-lang/rustc-dev-guide#1119)
- More specifics on what future-incompatible lints are used for
- Fix line lens
- Update information on lints particularly on future-incompatible
- Update section of lint store
- Update around half of the January 2021 date references (rust-lang/rustc-dev-guide#1155)
- Create issues for many TODOs (rust-lang/rustc-dev-guide#1163)
- Links from rustc-dev-guide to std-dev-guide (rust-lang/rustc-dev-guide#1152)
- Document how to mark features as incomplete (rust-lang/rustc-dev-guide#1151)
- Remove requests or suggestions about rebase and fixup contradictory to rust-highfive bot comment (rust-lang/rustc-dev-guide#1111)
- Generate glossary table correctly (rust-lang/rustc-dev-guide#1146)
- Correct the wrong serial number (rust-lang/rustc-dev-guide#1147)

## edition-guide

3 commits in c74b2a0d6bf55774cf15d69f05dfe05408b8f81a..5d57b3832f8d308a9f478ce0a69799548f27ad4d
2021-06-14 10:48:27 -0700 to 2021-07-05 10:33:32 +0200
- Add more info for warnings promoted to errors (rust-lang/edition-guide#247)
- Create triagebot.toml
- Clarify snippets in 2021 panic docs. (rust-lang/edition-guide#245)

## embedded-book

1 commits in cbec77fbd8eea0c13e390dd9eded1ae200e811d1..506840eb73b0749336e1d5274e16d6393892ee82
2021-06-10 06:26:32 +0000 to 2021-06-24 00:01:32 +0000
- Update book to track quickstart changes  (rust-embedded/book#296)
@jhg jhg deleted the issue_900 branch July 19, 2021 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

@JohnTitor JohnTitor JohnTitor approved these changes

Assignees
No one assigned
Labels
S-waiting-on-review Status: this PR is waiting for a reviewer to verify its content
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PR follow-up commits strategy at odds with comment from rust-highfive bot
3 participants
@jhg @jyn514 @JohnTitor