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

Add grammar in docs for {f32,f64}::from_str, mention known bug. #56217

Merged
merged 6 commits into from
Jan 25, 2019

Conversation

frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Nov 25, 2018

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 25, 2018
@frewsxcv
Copy link
Member Author

r? @rust-lang/docs @rust-lang/libs

/// [0-9]+)?)
/// ```
///
/// # Known bugs
Copy link
Member Author

Choose a reason for hiding this comment

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

Historically, we haven't mentioned known bugs in the docs, but considering this known bug has existed for years and people keep hitting it, I think it's worth calling it out here.

@frewsxcv frewsxcv changed the title Add grammar for {f32,f64}::from_str, mention known bug. Add grammar in docs for {f32,f64}::from_str, mention known bug. Nov 25, 2018
@frewsxcv frewsxcv force-pushed the frewsxcv-float-parse branch from b8b884c to d1c1978 Compare November 25, 2018 17:06
- Original bug about documenting grammar
  - rust-lang#32243
- Known bug with parsing
  - rust-lang#31407
@frewsxcv frewsxcv force-pushed the frewsxcv-float-parse branch from d1c1978 to 1798d51 Compare November 25, 2018 17:54
@rust-lang rust-lang deleted a comment from rust-highfive Nov 25, 2018
@rust-lang rust-lang deleted a comment from rust-highfive Nov 25, 2018
@alexcrichton
Copy link
Member

FWIW this isn't the first instance of documenting a known issue, and I think while we don't really have a policy around this the defacto policy is that "if someone cares enough to document the issue while not being willing to fix it, then seems fine to document".

@frewsxcv frewsxcv added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools labels Nov 29, 2018
@QuietMisdreavus
Copy link
Member

I'm not a fan of including a grammar or regular expression like that, especially in the std docs. We've gradually been moving any kind of grammar into the Reference, so i would consider that a much better place for that.

As far as documenting known issues, i'm okay with it, but i think it could be better phrased:

# Known issues

In some situations, some strings that should create a valid float instead return an
error. See [issue #31407] for details.

@frewsxcv
Copy link
Member Author

frewsxcv commented Dec 3, 2018

I'm not a fan of including a grammar or regular expression like that, especially in the std docs.

The point of this grammar is to document valid input formats for this particular standard library API. So I can get a better understanding of what you're envisioning, do you have any examples of sections in the Reference that document specific standard library APIs?

We've gradually been moving any kind of grammar into the Reference

Moving the Rust language syntax grammar to the Reference makes sense to me since it's documenting the behaviors of the language itself, as opposed to a specific standard library API

but i think it could be better phrased:

Thanks for the suggestion! I'll update it to reflect your wording

@QuietMisdreavus
Copy link
Member

do you have any examples of sections in the Reference that document specific standard library APIs?

There's a "Special types and traits" page that seems to document some of the more-common items from the standard library.

But i think i get your point. Since this is library code, it makes sense to document it here. (Though i'm not sure if we ever got together a better documentation of all type conversion functionality than the std::convert module docs, and that doesn't even include the FromStr trait being documented here...)

There is a grammar for floating-point literals in the reference; does this match the regular expression you've entered there? I guess it wouldn't include the type suffix, but i think everything else matches?

@frewsxcv
Copy link
Member Author

frewsxcv commented Dec 16, 2018

but i think everything else matches?

A few other differences come to mind:

  • float literals, like integer literals, can have underscores
  • float literals require a decimal point
  • float literals can't have a leading +

@frewsxcv
Copy link
Member Author

Also looks like float literals can have a capital E in the exponent part

@QuietMisdreavus
Copy link
Member

cc @rust-lang/wg-grammar - I think some people in there aren't in the docs group on github.

@Centril
Copy link
Contributor

Centril commented Dec 20, 2018

@QuietMisdreavus I'm not sure what the ping (wg-grammar) was for; can you elaborate?

@QuietMisdreavus
Copy link
Member

@Centril We talked about it on Discord, but my question was effectively "does the grammar WG care about the grammar of library functions", which turns out to be "not officially".

@frewsxcv Coming back to this, it's fine to have it here, but i find the regex a little hard to read. Can you convert it to BNF? I think that's a little easier to decode at a glance if you're not familiar with the syntax.

@frewsxcv
Copy link
Member Author

Here's my attempt at writing out the grammar using W3C's EBNF syntax:

Float  ::= Sign? ( 'inf' | 'NaN' | Number )
Number ::= ( Digit+ |
             Digit+ '.' Digit* |
             Digit* '.' Digit+ ) Exp?
Exp    ::= [eE] Sign? Digit+
Sign   ::= [+-]
Digit  ::= [0-9]

It can be visualized here. Thoughts? Is this more clear than the regex?

@Centril
Copy link
Contributor

Centril commented Dec 24, 2018

Picking someone from T-libs randomly...

r? @Kimundi

@frewsxcv
Copy link
Member Author

frewsxcv commented Jan 5, 2019

Anyone on the libs team have any concerns about this?

@jkordish
Copy link

ping from triage. @Kimundi have you had time to review?

@QuietMisdreavus
Copy link
Member

I think this looks good, thanks so much! Barring any objections from @rust-lang/libs, i think this is ready to go. I'll go ahead and push this forward, but feel free to back it out if there's a problem.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 24, 2019

📌 Commit 8af02fa has been approved by QuietMisdreavus

@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 Jan 24, 2019
Centril added a commit to Centril/rust that referenced this pull request Jan 25, 2019
…ietMisdreavus

Add grammar in docs for {f32,f64}::from_str, mention known bug.

- Original bug about documenting grammar
  - rust-lang#32243
- Known bug with parsing
  - rust-lang#31407
bors added a commit that referenced this pull request Jan 25, 2019
Rollup of 5 pull requests

Successful merges:

 - #56217 (Add grammar in docs for {f32,f64}::from_str, mention known bug.)
 - #57294 (When using value after move, point at span of local)
 - #57652 (Update/remove some old readmes)
 - #57802 (Print visible name for types as well as modules.)
 - #57865 (Don't ICE when logging unusual types)

Failed merges:

r? @ghost
@bors bors merged commit 8af02fa into rust-lang:master Jan 25, 2019
@frewsxcv frewsxcv deleted the frewsxcv-float-parse branch January 25, 2019 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants