Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[DNM] Comply
SymbolGraph.SemanticVersion
to SemVer 2.0.0 #36base: main
Are you sure you want to change the base?
[DNM] Comply
SymbolGraph.SemanticVersion
to SemVer 2.0.0 #36Changes from 1 commit
2dd562b
ff3e40f
4e2b309
b5f7003
ecbede7
1790ae6
4d9783d
c27b2bf
98a49de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the prerelease information on semantic was optional, then this could accept a non-optional String argument.
There was a problem hiding this 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 the internal representation of the pre-release information has much to do with this initializer signature. Rather, the reason that
dotSeparatedIdentifiers
is optional is because the pre-release string it's an optional information in the SymbolKit's OpenAPI documentation, and decoded as aString?
by SymbolKit. The only way to make it non-optional imo is moving the optional checking outside to right before the call site.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this comment: if the prerelease information on semantic was optional array of string then this initializer wouldn't need to check for nil values. Instead, if the prerelease string was
nil
then no Prerelease would be initialized. This could for example be done with optional map at the call site.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If prerelease was an optional value on SemanticVersion then these empty checks wouldn't be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but then we'd be trading an empty check for an optional check. We always have to check whether there is pre-release information regardless. Additionally, it will necessitate re-parsing (and maybe validating) the pre-release string every time on comparison.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm torn about about using
==
for this behavior. My concern is that they would accidentally get misused.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are some possible misuses you have in mind? As far as I can tell this is exactly what
==
is designed to do.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a developer thinks that two versions with different build metadata should be considered different but they are considered equal and that developer writes code based on that assumption that has bugs because the assumption is wrong I would consider that an unintended misuse of this equality behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Maybe a note in the documentation could help, maybe similar to how
FloatingPoint
does it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FloatingPoint
is a great example in that it's another well specified comparison behavior that has a few unintuitive cases. I'm sure many developers, myself included, would get some of the NaN behaviors wrong without looking them up. Especially the collection behaviors around sorting and filtering.In the
FloatingPoint
case it's clearly the right choice to use operators for equality and comparison but even then there have been discussions around some undesirable behaviors in the specification.Some of those considerations apply here as well. For example:
Documentation helps but I'm still thinking about if this would be better addressed at the API level by not using Equatable/Comparable for these behaviors. See this comment. In the FloatingPoint case it's clearly worth it but I feel that the semantic versions aren't as frequently compared as floating point numbers are and that semantic versions with build metadata would be more common than not-a-number floating point values. So that's why I'm a bit hesitant on this.