-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 std/optionals
; deprecate std/options
; alternative to #18401
#19828
Conversation
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.
Nice to see this finally being implemented! I also like the name std/optionals
.
While we're at it, can we also turn the proc
s into func
s?
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.
Instead of deprecating a module that is used by (I am guessing) most Nim code, how about we instead make this a breaking change for Nim v2?
We can still merge this for those that want this behaviour in 1.x, but maybe in that case we shouldn't deprecate the options
module?
Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
std/optionals
; deprecate std/options
std/optionals
; deprecate std/options
; alternative to #18401
For those of us that have significant codebases in Nim, changes to language and standard library changes that frivolously break our code and assumptions are deeply problematic - every time it happens, it represents a significant cost and constitutes a signal to other organizations to not make similar investments. Additive changes are the way to go. |
@arnetheduck yes, breaking changes are tough. But they are necessary eventually. Perhaps 2.0 is coming too soon, but do you agree that Nim eventually needs to evolve and leave its old baggage behind? Ideally we would keep Nim 1.x going and support it way past 2.0 to show that those who have invested in that ecosystem are not being left behind. |
Version 2.0 or not, if we can avoid breaking code we will. |
Wouldn't it be better to have 2 separate types in the |
In the vast majority of cases that have been suggested so far, no - they have been OCD-style "it should be this way because I like the name better or dislike the existing name" and not fundamental. Where fundamental breaking changes are necessary we've established a way forward for those as well, and not limited to 2.0 (ie provide a better alternative for a period of time, switch default, the finally remove) - the impetus for 2.0 is that it comes with a new memory manager which requires people to learn a new style of code because "canonical and well-written Nim" no longer looks the same similar to when C++11 came along and changed the C++ landscape and how C++ is written. Notably though, C++ introduced very few actual breaking changes in that transition - there were a few but mostly, the changes were done in an additive manner, which is entirely possible for the vast majority of suggested changes, if you understand the concept that every breaking change decreases the amount of actually working Nim code out there - if adoption is the goal, a vibrant ecosystem of libraries is necessary, and breaking the existing ones is not a good way to get there, really. The difficult to introduce change and maintain compatibility at the same time is a direct outcome of having a large standard library - for the ecosystem around Nim, this is not a problem really: we can release a new better version and that's it. Nim itself, and its library however, is unique in its ability to break everything out there, sitting at the root of every dependency tree and therefore, it pays of to be more careful. |
To elaborate, when upgrading a large codebase, it is often not possible to upgrade everything at once: different libraries have different schedules, requirements, manpower availability and so on - this basically means that if a project is composed of 20 libraries, its not feasible that all of these libraries adapt to the change at the same time - rather, it's a gradual process where both the new and the old world needs to be supported by each library, and when all are ready, the switch can be made. A breaking change to Nim makes it impossible to perform this incremental transition - it took python 15 years of debate and pain - I don't think it's in Nim's interest to maintain this kind of transition as it is onerous for everyone involved - using techniques like those described (additive changes, or overlapping compatibility periods etc), change can happen without the drama. |
That may be true, but as I understand it I do take the point that we should provide transition periods, from that perspective creating a new module and hinting to developers to transition to it is wise (via deprecation). My only concern is the threshold at which we create such new modules, if we do so for any small breaking change then we will soon end up with a range of duplicate modules, something which for Nim developers (especially newcomers and those that cannot pay attention to all the changes) will be tough to deal with. Thinking about it some more: maybe a better approach in this case and other similar cases would be to introduce an opt-in flag with a warning to transition to it, instead of duplicating the module. Hm, but then we have the problem that the flag applies to the full application, not just a specific package. Just thinking out loud :) |
This needs to be implemented differently. The impl must use a |
assert optional(42).isSome | ||
when T is SomePointer: | ||
result.val = val | ||
result.has = val != nil |
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.
An Optional[ptr]
has 3 states: unset, set to nil and set to some valid pointer - how do I set it to nil
?
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.
some(nil)
IMO those are the minimum functions for |
result.val = val | ||
result.has = true | ||
|
||
proc some*[T](val: sink T): Optional[T] {.inline.} = |
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.
proc some*[T](val: sink T): Optional[T] {.inline.} = | |
proc some*[T](O: type Optional, val: sink T): Optional[T] {.inline.} = |
the above signature will make the syntax Optional.some(value)
which may be an advantage when wanting to avoid name clashes
I've had a little-thought-out, probably unproductive opinion about this for a while and I've decided this being tangentially brought up in a forum post just now is a good enough excuse to say it. I don't think we need an object form of One could say we can wait for Calling it "Option" or attaching FP to it is definitely misleading in this case though. And I don't think it's worth the effort of coming up with new optics for it. So I am fine with conceding and making Options centered around FP, however I don't think this will result in much adoption in the standard library. |
This isn't about having a type
Optional[T] = object
case has: bool
of true:
val: T
of false:
discard
A common use case for an optional type is as a return type for some value that may or may not exist. If "the element doesn't exist" is indicated by
I don't see how
Why is "Option" misleading? That's exactly what it is and it works just like in FP.
I don't think there's any evidence for this, there have been several times where people have wanted to use optional types in the stdlib, e.g. #18359. |
I'd second that providing table1.get("name").map() do(x: int):
echo "x: ", x Not perfect but fits in with regular sort/map/etc like in the do notation docs. |
It seems like the old |
ref #18401 (comment)