Skip to content

Conversation

@jakobnissen
Copy link
Member

Anecdotally, it is not always clear for beginners that Int and Int64 is the same type, and this leads to some confusion for newcomers. This is described in the manual, but it would be nice to also have it in the docstring.

Furthermore, mention that Int is used as the default integer type in Julia. Where some languages use UInt for things such as array sizes, Julia nearly universally uses Int as the integer type.

@giordano giordano added the docs This change adds or pertains to documentation label May 22, 2022
@mcabbott
Copy link
Contributor

See also #45221 which mentions that there is this default for Int (although not at present for UInt)

@jakobnissen
Copy link
Member Author

CI failure seems unrelated.

end

"""
$(Int) <: Signed
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$(Int) <: Signed
Int <: Signed

Isn't more reasonable if we hardcode this?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is that it will show up as e.g. Int64 on 64-bit systems, not Int, since Int64 is the canonical name of the type, and Int is an alias.

@mcabbott
Copy link
Contributor

mcabbott commented Jun 1, 2022

If this is approved, can you clarify what you envisage happening with the earlier #45221? That also aims to explain the Int default, but does not make a separate Int docstring, as I thought that would overwrite the one for Int64.

@aviatesk
Copy link
Member

aviatesk commented Jun 1, 2022

It seems like we can access to both documentations even if we have a separate documentation for Int:

julia> @eval begin
           """
               Parent
       
           Documentation for `Parent`
           """
           struct Parent end
       
           """
               Aliased
       
           Documentation for `Aliased`
           """
           const Aliased = Parent
       end
Aliased

help?> Parent
search: Parent parent parentmodule parentindices

  Parent


  Documentation for Parent

help?> Aliased
search: Aliased

  Aliased


  Documentation for Aliased

@jakobnissen
Copy link
Member Author

jakobnissen commented Jun 1, 2022

This is from a fresh build off this branch. The docstring only appears once - which I think if preferrable. I don't understand why, when it appears twice in your example.

Edit: Now I get two different docstrings. Not sure what changed.

image

@mcabbott
Copy link
Contributor

mcabbott commented Jun 1, 2022

I thought the example was showing that Int and Int64 would access two different docstrings.

That seems a little confusing to me, what details should appear in one and not the other? I would never think to read both, when Int === Int64.

@jakobnissen
Copy link
Member Author

Oh sorry, you are right! I did not understand Shuhei's example. Yeah, I agree, this is not what I want here. There should only be one docstring, Int64, and accessing Int should display that.
I'll fix it shortly.

@ViralBShah
Copy link
Member

Is this good to merge? I am rebasing it to master to be able to run CI (which was quite broken the last time it ran here).

@jakobnissen
Copy link
Member Author

@ViralBShah I see there are some CI failures with building the docs. I'll iron them out, then ping you when tests pass and it can be merged

@jakobnissen
Copy link
Member Author

@ViralBShah After having skimmed them, I think the numerous CI failures are all unrelated to this change. This is ready to merge.

@ViralBShah
Copy link
Member

@giordano @mcabbott Since you guys have both chimed in - do you have any thoughts on merging?

Co-authored-by: Allen Hill <halleysfifthinc@users.noreply.github.com>
@mcabbott
Copy link
Contributor

It still seems confusing to me to have two docstrings for Int and Int64 when these are ===. I have not built this branch, and am not quite sure whether the claim above is that the two may both be accessed (as in the example code) which is confusing, or whether one is completely overwritten. But if completely overwritten, then it still seems a confusing way to write the source code, like defining a function twice & expecting the reader to know which will win. I thought this comment indicated an intention to remove one of them, but as far as I can see the code at present still contains two.

I also still wonder what the plan is with the earlier PR #45221, which also expands the docstring for Int64 to mention the alias Int. It alters the loop which defines this and Int32 to add the mention to one of the two. My vote would be to just add whatever details are missing from that, there or in a follow-up PR.

@fingolfin
Copy link
Member

Since PR #45221 has been merged, there are docstrings for UInt and Int. So I think this PR can be closed.

(If you'd like to tweak the new docstrings, I'd recommend opening a new PR to make sure it gets proper attention)

@fingolfin fingolfin closed this Feb 13, 2024
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.

7 participants