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

add more docs regarding ideographic numbers #99628

Merged
merged 1 commit into from
Jul 28, 2022
Merged

add more docs regarding ideographic numbers #99628

merged 1 commit into from
Jul 28, 2022

Conversation

vincenzopalazzo
Copy link
Member

This was discussed in the last lib meeting and I try to avoid forgetting to open a PR because I think having some docs can help people.

However, I think we need to discuss a little bit if this is enough or if we need to add more clarification? Maybe an example?

Inspiration Source: #84056 (comment)

Including suggestion #99626 (comment) my bad command git close the PR

@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 22, 2022
@rust-highfive
Copy link
Collaborator

r? @scottmcm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2022
@vincenzopalazzo vincenzopalazzo mentioned this pull request Jul 22, 2022
Closed
@the8472
Copy link
Member

the8472 commented Jul 23, 2022

The code sample could also be updated to include a few number-like things that aren't detected as numeric.

@vincenzopalazzo
Copy link
Member Author

I added your suggestion @the8472 and I think your description is really good to put inside the docs, I have no better way to explain it, maybe if there are any suggestions on the form I can apply them.

I also add an example of a number, and I'm thinking to add a comment like // this is a number in Chinese, and it is evaluated to false because for me it is just another symbol. But not sure that this brings value to the docs.

@the8472
Copy link
Member

the8472 commented Jul 26, 2022

I also add an example of a number, and I'm thinking to add a comment like // this is a number in Chinese, and it is evaluated to false because for me it is just another symbol.

Perhaps 一, 二, 三 are more intuitive examples even when one is not familiar with chinese characters. The comment can also be a bit shorter like

// chinese numerals are not categorized as numeric

@vincenzopalazzo
Copy link
Member Author

Zero knowledge of Chinese so I put a better char example :) thanks

@est31
Copy link
Member

est31 commented Jul 26, 2022

For @the8472 's concern, I would suggest "This means that" instead of "However". Also it might be good to add a short explanation for what ideographic numbers are. Readers usually aren't unicode experts, know languages that have ideographic numbers, or have read #84056 .

Mh! I agree, but what is the best definition of "ideographic numbers"?

@vincenzopalazzo already adding an example that clarifies what ideographic number is would be nice. Just change it to "ideographic numbers like '三'".

@vincenzopalazzo
Copy link
Member Author

Added also this clarification @est31 thanks!

@joshtriplett
Copy link
Member

@vincenzopalazzo This most recent change seems to have lost the much more detailed version you had before.

@scottmcm
Copy link
Member

Since it looks like you're already keeping up with this PR and the meeting discussions about the issue,
r? @joshtriplett

@vincenzopalazzo vincenzopalazzo force-pushed the macros/is_number_doc branch 2 times, most recently from f19af32 to 8c33531 Compare July 28, 2022 09:13
@vincenzopalazzo
Copy link
Member Author

@joshtriplett Thanks to catch this, I think my remote editor was outdate and did not get update with the last version of the git checkout, but it was my fault because I did not make a double check.

again, thanks!

@vincenzopalazzo
add more docs regarding ideographic numbers
47a0a56 
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@joshtriplett
Copy link
Member

@bors r+ rollup

@joshtriplett
Copy link
Member

Thanks for working on this!

@bors
Copy link
Contributor

bors commented Jul 28, 2022

📌 Commit 47a0a56 has been approved by joshtriplett

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 28, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Jul 28, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 28, 2022
@bors
Auto merge of rust-lang#99863 - Dylan-DPC:rollup-lq9w047, r=Dylan-DPC
9067d52 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#99628 (add more docs regarding ideographic numbers)
 - rust-lang#99689 (Revert `write!` and `writeln!` to late drop temporaries)
 - rust-lang#99807 (Fix PermissionDenied UI tests on WSL)
 - rust-lang#99817 (rustdoc: remove Clean trait impls for more items)
 - rust-lang#99851 (Fix small typo in Cargo.toml comment)
 - rust-lang#99856 (fix: remove fake no_dead_strip for osx)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a479cab into rust-lang:master Jul 28, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 28, 2022
@vincenzopalazzo vincenzopalazzo mentioned this pull request Aug 9, 2022
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 10, 2022
@matthiaskrgr
Rollup merge of rust-lang#100345 - vincenzopalazzo:macros/is_number_d…
3b97de6 
…oc, r=joshtriplett

docs: remove repetition in `is_numeric` function docs

In rust-lang#99628 we introduce new docs for the `is_numeric` function, and this is a follow-up PR that removes some unnecessary repetition that may be introduced by some rebasing.

`@rustbot` r? `@joshtriplett`
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
@matthiaskrgr
Rollup merge of #100345 - vincenzopalazzo:macros/is_number_doc, r=jos…
a61d823 
…htriplett

docs: remove repetition in `is_numeric` function docs

In rust-lang/rust#99628 we introduce new docs for the `is_numeric` function, and this is a follow-up PR that removes some unnecessary repetition that may be introduced by some rebasing.

`@rustbot` r? `@joshtriplett`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@est31 est31 est31 left review comments

@the8472 the8472 the8472 left review comments

@joshtriplett joshtriplett joshtriplett left review comments

Assignees

@joshtriplett joshtriplett

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.64.0
Development

Successfully merging this pull request may close these issues.
8 participants
@vincenzopalazzo @rustbot @rust-highfive @the8472 @est31 @joshtriplett @scottmcm @bors