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

Document the fields of VersionNumber #50179

Merged
merged 2 commits into from
Nov 2, 2023
Merged

Conversation

jakobnissen
Copy link
Contributor

@jakobnissen jakobnissen commented Jun 15, 2023

VersionNumber's fields are currently not documented, and therefore, private by default.
However, if people want to use VersionNumber for anything other than the documented comparison operations, users will have to access the fields.

Questions for reviewers

Do we want this? Normally, fields are not documented for a reason. But I feel like it's hard to use VersionNumber for much in practice without access to its fields, unless we add several more methods such as major(::VersionNumber) etc.

Should we change the field type of .prerelease and .build from Tuple{Vararg{T}} to Vector{T}? It seems like it was changed from Vector to Tuple in ef69a19 to make VersionNumber immutable. I'm not sure why that's a good thing, since it currently means accessing .prerelease is type unstable but there might be a good reason.

@oscardssmith
Copy link
Member

I believe immutability allows for better compile time comparisons. Is there a case where there's a length greater than 1?

@jakobnissen
Copy link
Contributor Author

Yes, the two fields can have any number of prerelease or build tags separated by a comma.

@brenhinkeller brenhinkeller added the docs This change adds or pertains to documentation label Aug 6, 2023
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Oct 31, 2023
test/version.jl Outdated
@@ -219,6 +219,12 @@ for major=0:3, minor=0:3, patch=0:3
end
end

# banner
import Base.banner
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't exist anymore on master

Copy link
Contributor Author

@jakobnissen jakobnissen Nov 1, 2023

Choose a reason for hiding this comment

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

Must have been a rebasing problem. Fixed, thanks!

`VersionNumber`'s fields are currently not documented, and therefore, private
by default.
However, if people want to use `VersionNumber` for anything other than the
documented comparison operations, users will have to access the fields.
@jakobnissen
Copy link
Contributor Author

OK - rebased and ready.
There is still the issue that the field type Tuple{Vararg{Union{Integer, AbstractString}}} is documented - even though this is an abstract type. Should we perhaps either

  • Change it to be more vague, e.g. say "it's an iterable of Union{Integer, AbstractString}"
  • Actually change the struct field to Vector
    If not, just merge it as-is.

@vtjnash vtjnash merged commit dabb93a into JuliaLang:master Nov 2, 2023
5 of 7 checks passed
@jakobnissen jakobnissen deleted the vnum_docs branch November 2, 2023 14:29
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Nov 4, 2023
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.

6 participants