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
Add "Make .Is* discriminated union properties visible from F#" #402
Add "Make .Is* discriminated union properties visible from F#" #402
Changes from 1 commit
7d0ec73
41dd0ab
506f0f2
3803ffa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This isn't true for
Option<'T>
orValueOption<'T>
. See here.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.
Option<'T>
is easy:IsSome
andIsNone
are already visible, so there is no change.For
ValueOption<'T>
, there are also visibleIsSome
andIsNone
, in addition to the hiddenIsValueSome
andIsValueNone
. So there are two possible routes:IsSome
/IsNone
)ValueOption<'T>
unique among all union types for not havingIs{{UnionCaseName}}
properties)I think I'm in favor of the former, for consistency. Plus it's probably simpler to implement than making an exception.
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 assume we can't remove the existing
IsSome/None
, but addingIsValueSome/None
would effectively make them aliases of the former, in which they would then probably be unique in our ecosystem.It's a tough call. For parity you would like to have them, but different members that do the exact same thing brings confusion, and unless the tooltip would clearly describe that they are synonyms, I'm afraid programmers will have to research what the differences are and you'll end up answering questions similar to whether
String
orstring
has better performance / should be preferred / has different semantics etc.Of course, the answers will be simple, they're equal, but I think it's better to prevent the questions rising in the first place.
But then there's the argument of parity, auto generated code, predictability, orthogonality etc. It's a tough call ;).
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 not sure what the exact stance on obsoleting API is, but it could be acceptable to mark the existing ValueOption IsSome/IsNone properties
[<Obsolete>]
to reduce confusion.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.
@Tarmil I think your preference here is spot on: hiding
IsValueSome
/IsValueNone
for ValueOption. Can you make a note of that in the RFC?