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

Fix inconsistent highlight blocks. #62951

Closed
wants to merge 4 commits into from
Closed

Fix inconsistent highlight blocks. #62951

wants to merge 4 commits into from

Conversation

tomasz-rozanski
Copy link
Contributor

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 24, 2019
@tomasz-rozanski tomasz-rozanski mentioned this pull request Jul 24, 2019
Closed
@shepmaster
Copy link
Member

I apologize; I wasn't very clear in the other issue. I think that only the Unicode properties/categories should be in code blocks. This documentation uses single quotes / apostrophes (') as a quoting mechanism, emphasizing a specific term. That's a valid usage.

I'll attempt to comment with the lines that should not be changed.

shepmaster
shepmaster reviewed Jul 24, 2019
View reviewed changes
Copy link
Member

@shepmaster shepmaster left a comment

Choose a reason for hiding this comment

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

It was easier to mark the ones to keep, except a few places where I wanted to be extra precise.

src/libcore/char/methods.rs
@@ -547,10 +547,10 @@ impl char {
}
}

/// Returns `true` if this `char` satisfies the 'XID_Start' Unicode property, and false
/// Returns `true` if this `char` satisfies the `XID_Start` Unicode property, and false
Copy link
Member

Choose a reason for hiding this comment

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

Keep

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's clearer when reading the documentation to highlight this with backticks and it does refer to something which is code-like. At least if not with backticks it would be good to use 'XID_Start' to highlight more.

Copy link
Member

Choose a reason for hiding this comment

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

"Keep" means "keep the change to backticks".

src/libcore/char/methods.rs
/// otherwise.
///
/// 'XID_Start' is a Unicode Derived Property specified in
/// `XID_Start` is a Unicode Derived Property specified in
Copy link
Member

Choose a reason for hiding this comment

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

Keep

src/libcore/char/methods.rs
@@ -563,12 +563,12 @@ impl char {
derived_property::XID_Start(self)
}

/// Returns `true` if this `char` satisfies the 'XID_Continue' Unicode property, and false
/// Returns `true` if this `char` satisfies the `XID_Continue` Unicode property, and false
Copy link
Member

Choose a reason for hiding this comment

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

Keep

src/libcore/char/methods.rs
/// otherwise.
///
/// 'XID_Continue' is a Unicode Derived Property specified in
/// `XID_Continue` is a Unicode Derived Property specified in
Copy link
Member

Choose a reason for hiding this comment

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

Keep

src/libcore/char/methods.rs
/// [UAX #31](http://unicode.org/reports/tr31/#NFKC_Modifications),
/// mostly similar to 'ID_Continue' but modified for closure under NFKx.
/// mostly similar to `ID_Continue` but modified for closure under NFKx.
Copy link
Member

Choose a reason for hiding this comment

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

Keep

src/libcore/char/methods.rs Outdated
@@ -665,8 +665,8 @@ impl char {

/// Returns `true` if this `char` is alphanumeric.
///
/// 'Alphanumeric'-ness is defined in terms of the Unicode General Categories
/// 'Nd', 'Nl', 'No' and the Derived Core Property 'Alphabetic'.
/// `Alphanumeric`-ness is defined in terms of the Unicode General Categories
Copy link
Member

Choose a reason for hiding this comment

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

Revert

src/libcore/char/methods.rs
/// 'Alphanumeric'-ness is defined in terms of the Unicode General Categories
/// 'Nd', 'Nl', 'No' and the Derived Core Property 'Alphabetic'.
/// `Alphanumeric`-ness is defined in terms of the Unicode General Categories
/// `Nd`, `Nl`, `No` and the Derived Core Property `Alphabetic`.
Copy link
Member

Choose a reason for hiding this comment

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

Keep

src/libcore/char/methods.rs Outdated
@@ -719,8 +719,8 @@ impl char {

/// Returns `true` if this `char` is numeric.
///
/// 'Numeric'-ness is defined in terms of the Unicode General Categories
/// 'Nd', 'Nl', 'No'.
/// `Numeric`-ness is defined in terms of the Unicode General Categories
Copy link
Member

Choose a reason for hiding this comment

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

Revert

src/libcore/char/methods.rs
/// 'Numeric'-ness is defined in terms of the Unicode General Categories
/// 'Nd', 'Nl', 'No'.
/// `Numeric`-ness is defined in terms of the Unicode General Categories
/// `Nd`, `Nl`, `No`.
Copy link
Member

Choose a reason for hiding this comment

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

Keep

@tomasz-rozanski
Copy link
Contributor Author

So all the changes you didn't quote above, I should also revert?

@shepmaster
Copy link
Member

So all the changes you didn't quote above, I should also revert?

Yes please. Basically, the things that should be in backticks are the terms specifically referring to a specific Unicode concept.

@tomasz-rozanski
Copy link
Contributor Author

I think I nailed it this time. Fingers crossed.

@Centril
Copy link
Contributor

Centril commented Jul 25, 2019

Merge remote-tracking branch 'upstream/master'

Can you please squash the commits? We prefer not to have merge commits in our history.

@tomasz-rozanski
Copy link
Contributor Author

Merge remote-tracking branch 'upstream/master'

Can you please squash the commits? We prefer not to have merge commits in our history.

I'm sorry, but it seems that I can't do this anymore. I tried to rebase my local repository, but with no success. I also couldn't find that kind of option on GitHub.

I'm pretty new to Git, so maybe I'm missing something obvious.

@Centril
Copy link
Contributor

Centril commented Jul 25, 2019

@Rosto75 Can you try git rebase -i origin/master and then use "fixup" on the 2nd and 3rd commits? Once you've done that you can git push --force-with-lease to push the changes.

@tomasz-rozanski
Copy link
Contributor Author

@Rosto75 Can you try git rebase -i origin/master and then use "fixup" on the 2nd and 3rd commits? Once you've done that you can git push --force-with-lease to push the changes.

After calling git rebase the todo-list file contained only noop. I tried to populate it manually, by putting:

pick ab2ff17b0bad3a9409b51c2c1ed44abe183b5e64
fixup 4c501d19e598c9e44177184bf9c754e7096206bb

and then tried with squash:

pick ab2ff17b0bad3a9409b51c2c1ed44abe183b5e64
squash 4c501d19e598c9e44177184bf9c754e7096206bb

No visible effects, after pushing it.

In the meantime, I managed to create an extra, empty commit, so it's even a bigger mess now. Maybe I should start from scratch.

@tomasz-rozanski
Copy link
Contributor Author

Third time's a charm #62977
🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centril Centril Centril left review comments

@shepmaster shepmaster shepmaster left review comments

Assignees

@shepmaster shepmaster

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tomasz-rozanski @shepmaster @Centril @rust-highfive