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

Use qualified type reference YAML::Any #12688

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

zw963
Copy link
Contributor

@zw963 zw963 commented Oct 29, 2022

Current generated document not accurately.

image

this PR make those more accurately.

@straight-shoota
Copy link
Member

What's the problem? I don't see any inaccuracy. Any within the YAML/JSON namespaces refers to YAML::Any/JSON::Any, respectively.

@zw963
Copy link
Contributor Author

zw963 commented Oct 29, 2022

What's the problem? I don't see any inaccuracy. Any within the YAML/JSON namespaces refers to YAML::Any/JSON::Any, respectively.

newbie user for use YAML.parse don't know what is the Any means, tell the user exactly #as_a will return Array(YAML::Any) is better than only Array(Any), right?

Analysis from another angle, if you want return a JSON::Any from some convert methods defined in YAML, what is the Any stand for?

@asterite
Copy link
Member

You can click Any and you will know.

@zw963
Copy link
Contributor Author

zw963 commented Oct 29, 2022

You can click Any and you will know.

Okay, click jump to same position.

So, Do we consider give uniform format is more neatly? (check following screenshot)

image

@asterite
Copy link
Member

I mean, if you want the fully qualified name then it's a doc generator change. But types lookup in code works the same as in docs. If you know one you should understand the other.

@zw963
Copy link
Contributor Author

zw963 commented Oct 29, 2022

if you want the fully qualified name then it's a doc generator change

@asterite, although titled as document improvement, but that not the only reason:

  1. Use same style(all qualified or all not qualified) is better, for std-lib, if let me choice, i always prefer later, why not all use same form?
  2. qualified name will result in a better performance? not sure, i means, ::YAML::Any is fairly straightforward, don't need lookup Any belong to which namespace, right?

@straight-shoota
Copy link
Member

straight-shoota commented Nov 25, 2022

why not all use same form?

Writing out the fully qualified name of every constant adds a lot of clutter and makes the code hard to read. In the context of JSON namespace, it should be sufficiently clear that Any refers to JSON::Any, not any of the other anys out there.

qualified name will result in a better performance? not sure, i means, ::YAML::Any is fairly straightforward, don't need lookup

Even a fully qualified name needs lookup of the name spaces for validation. I won't comment on any performance consideration of either variant because it's absolutely negligible. It doesn't make any difference.

This is entirely a question of developer ergonomics, considerations of compiler performance must not affect that (b/c it doesn't matter).

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

This is bringing consistency in the code and docs. Thanks @zw963 !

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

We talked about this issue and agreed that for the specific case of Any it makes sense to use qualified names because it's easy to get confused about it.

@straight-shoota straight-shoota added this to the 1.7.0 milestone Nov 30, 2022
@straight-shoota straight-shoota changed the title Refine any type Use qualified type reference YAML::Any Dec 1, 2022
@straight-shoota straight-shoota merged commit 251bd55 into crystal-lang:master Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants