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

Improve module description for std::f32 and std::f64 #41122

Merged
merged 1 commit into from
Apr 14, 2017
Merged

Improve module description for std::f32 and std::f64 #41122

merged 1 commit into from
Apr 14, 2017

Conversation

palango
Copy link
Contributor

@palango palango commented Apr 6, 2017

Fixes #29353, see discussion there.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (or someone else) soon.

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.

Please see the contribution instructions for more information.

@BurntSushi
Copy link
Member

@palango Thanks!

I'm not quite sure the docs here are enough to satisfy #29329.

It might be worth saying more about the distinction of constants defined in std::{f32,f64} and std::{f32,f64}::consts. For example, something like, "constants in this module are specific to the floating point implementation while constants in the consts sub-module contain mathematically significant numbers."

cc @steveklabnik @rust-lang/libs

@BurntSushi BurntSushi added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 6, 2017
@frewsxcv frewsxcv added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Apr 9, 2017
frewsxcv
frewsxcv reviewed Apr 10, 2017
View reviewed changes
src/libstd/f32.rs Outdated
//! This module provides access to properties and bounds of the 32-bit
//! floating point data type.
//! Mathematical constants are provided in the
//! [`consts`](../f32/consts) module.
Copy link
Member

Choose a reason for hiding this comment

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

I think this URL should just be:

...provided in the [`consts`][consts] module.

or better yet:

...provided in the [`consts`] module.

[`consts`]: consts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint.

@palango
Copy link
Contributor Author

palango commented Apr 10, 2017

@BurntSushi Thanks for your comments. I updated the description and think it's clearer now.

@palango palango force-pushed the mod-desc-floats branch 2 times, most recently from 37ed317 to 5d6f4f7 Compare April 11, 2017 17:52
@palango
Copy link
Contributor Author

palango commented Apr 11, 2017

Ok, I need some tips for getting the links to work.

If the link is done with [consts]: consts then I get the following error:

[00:43:58] std/f32/index.html:54: directory link - std/f32/consts
[00:43:58] std/index.html:452: broken link - std/consts
[00:43:58] std/index.html:462: broken link - std/consts
[00:44:00] std/f64/index.html:54: directory link - std/f64/consts

If I use [consts]: {f32|f64}/consts I get the following output, but one of of both always seems to be wrong.

[00:42:03] std/f32/index.html:54: broken link - std/f32/f32/consts
[00:42:03] std/index.html:452: directory link - std/f32/consts
[00:42:03] std/index.html:462: directory link - std/f64/consts
[00:42:05] std/f64/index.html:54: broken link - std/f64/f64/consts

@frewsxcv
Copy link
Member

Ah, this is an example where linking doesn't always work. It looks like these links get rendered on different pages that live at different levels of the URL hierarchy. We're working on fixing this and making this easier in the future. Sorry for the confusion here, you can just remove the links for now @palango. Thanks for being patient! :)

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2017
@frewsxcv
Copy link
Member

Thanks! @bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 14, 2017

📌 Commit 4b4b1e1 has been approved by frewsxcv

@frewsxcv frewsxcv 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 Apr 14, 2017
@bors
Copy link
Contributor

bors commented Apr 14, 2017

⌛ Testing commit 4b4b1e1 with merge 5637ed7...

bors added a commit that referenced this pull request Apr 14, 2017
@bors
Auto merge of #41122 - palango:mod-desc-floats, r=frewsxcv
5637ed7 
Improve module description for std::f32 and std::f64

Fixes #29353, see discussion there.
@bors
Copy link
Contributor

bors commented Apr 14, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: frewsxcv
Pushing 5637ed7 to master...

@bors bors merged commit 4b4b1e1 into rust-lang:master Apr 14, 2017
@frewsxcv frewsxcv mentioned this pull request Apr 14, 2017
Closed
1 task
@palango palango deleted the mod-desc-floats branch June 9, 2022 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frewsxcv frewsxcv frewsxcv left review comments

Assignees

@BurntSushi BurntSushi

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@palango @rust-highfive @BurntSushi @frewsxcv @bors @Mark-Simulacrum