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

doc improvements #22289

Merged
merged 1 commit into from
Jun 25, 2017
Merged

doc improvements #22289

merged 1 commit into from
Jun 25, 2017

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Jun 8, 2017

some random doctests, moving things from Base.jl

plz no goalpost moving

@KristofferC KristofferC added backport pending 0.6 docs This change adds or pertains to documentation labels Jun 8, 2017
@tkelman
Copy link
Contributor

tkelman commented Jun 8, 2017

more specific commit message

base/parse.jl Outdated
0.0012
```
"""
function parse(T::Type, str, base=Int) end
Copy link
Contributor

Choose a reason for hiding this comment

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

=Int doesn't make much sense here

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

looks wrong there too

Copy link
Member Author

Choose a reason for hiding this comment

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

What should it be? ::Int?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it was a manual mistake in 9e6aecc that no one noticed

In the 3-arg method with a base it's constrained to be ::Integer. In the 2-arg method below 0 gets passed instead of check_valid_base(base), but a comment says # Zero means, "figure it out" which isn't consistent with "the default is 10" from the docstring. parse(Int, "0b01", 10) errors and parse(Int, "0b01") doesn't...

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I believe the syntax here is wrong. This defines a function.

@KristofferC
Copy link
Member Author

KristofferC commented Jun 8, 2017

Please suggest a more specific commit message. Could be fixed on squash.

@tkelman
Copy link
Contributor

tkelman commented Jun 8, 2017

"added doctests for ..., moved docstrings for ... inline"

base/parse.jl Outdated
0.0012
```
"""
function parse(T::Type, str, base=Int) end
Copy link
Contributor

Choose a reason for hiding this comment

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

Also I believe the syntax here is wrong. This defines a function.

@KristofferC
Copy link
Member Author

Will rebase after #22515

@tkelman
Copy link
Contributor

tkelman commented Jun 25, 2017

This is smaller and ready, please do the other way around. Let people review things by leaving them open at least a day or so if there's no urgent rush.

@KristofferC
Copy link
Member Author

Let people review things by leaving them open at least a day or so if there's no urgent rush.

What is this referring to?

@KristofferC
Copy link
Member Author

Anyway, this is ready to go as far as I am concerned.

@tkelman
Copy link
Contributor

tkelman commented Jun 25, 2017

after #22515

@tkelman tkelman merged commit d9d8223 into master Jun 25, 2017
@tkelman tkelman deleted the kc/docs branch June 25, 2017 06:05
@KristofferC
Copy link
Member Author

There wouldn't be any point rebasing it before #22515?

DrTodd13 pushed a commit to IntelLabs/julia that referenced this pull request Jun 26, 2017
ararslan pushed a commit that referenced this pull request Sep 11, 2017
ararslan pushed a commit that referenced this pull request Sep 13, 2017
vtjnash pushed a commit that referenced this pull request Sep 14, 2017
ararslan pushed a commit that referenced this pull request Sep 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants