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 some string parsing macros #28085

Merged
merged 6 commits into from
May 9, 2022
Merged

Doc some string parsing macros #28085

merged 6 commits into from
May 9, 2022

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Jul 12, 2018

After my silly self deleted the old branch...

@kshyatt kshyatt added docs This change adds or pertains to documentation strings "Strings!" labels Jul 12, 2018
@kshyatt kshyatt requested a review from StefanKarpinski July 12, 2018 17:47
base/int.jl Outdated
julia> @int128_str "123456789123"
123456789123

julia> @int128_str "123456789123.4"
Copy link
Member

@fredrikekre fredrikekre Jul 12, 2018

Choose a reason for hiding this comment

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

Use the int128"1234" syntax here and in the other examples since that is how they are used in 99% of the cases?

Copy link
Member

@fredrikekre fredrikekre left a comment

Choose a reason for hiding this comment

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

I don't think it is worth showing the @int128_str "123" form, since the _str suffix is kind of an implementation detail, and it is never used that way.

base/int.jl Outdated

# Examples
```jldoctest; filter = r"Stacktrace:(\\n \\[[0-9]+\\].*)*"
julia> @int128_str "123456789123"
Copy link
Member

Choose a reason for hiding this comment

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

Is this form even worth showing?

Copy link
Member

@KristofferC KristofferC Jul 13, 2018

Choose a reason for hiding this comment

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

Same with the other ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is worth showing just because the doc string has that format. It's not very long...

Copy link
Member

Choose a reason for hiding this comment

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

I disagree; the _str is an ugly implementation detail that is not necessary to show. It is IMO better to say that this is a string macro, and reference that section in the manual, where interested users can read more about the implementation.

because the doc string has that format

I think we can change it to

"""
    int128"str"

Parse `str` as ...
"""

which looks better, and is how you would encounter in IRL.

base/int.jl Outdated
julia> @int128_str "123456789123.4"
ERROR: LoadError: ArgumentError: invalid base 10 digit '.' in "123456789123.4"
Stacktrace:
[1] tryparse_internal(::Type{Int128}, ::String, ::Int64, ::Int64, ::Int64, ::Bool) at ./parse.jl:118
Copy link
Member

Choose a reason for hiding this comment

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

Remove this stackframe?

@Keno
Copy link
Member

Keno commented Jan 17, 2020

Worth mentioning that these are emitted by the parser for long numeric literals.

@fingolfin
Copy link
Member

@kshyatt any plans to rebase and update this? Would be really nice :-)

doc/src/base/numbers.md Outdated Show resolved Hide resolved
@vtjnash
Copy link
Member

vtjnash commented Apr 20, 2021

Looks like #28719 and #27949 already included much of this. Though could still add Keno's comment:

Worth mentioning that these are emitted by the parser for long numeric literals.

base/int.jl Outdated Show resolved Hide resolved
ViralBShah and others added 2 commits November 16, 2021 19:37
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@KristofferC KristofferC added the merge me PR is reviewed. Merge when all tests are passing label May 6, 2022
@KristofferC KristofferC merged commit a86c687 into master May 9, 2022
@KristofferC KristofferC deleted the ksh/docmacros branch May 9, 2022 12:56
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label May 13, 2022
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 strings "Strings!"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants