-
Notifications
You must be signed in to change notification settings - Fork 3
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
Remove Questionable #35
Conversation
or "why Questionable is awesome"
instead of std/options (which has fundamental issues / will be replaced soon or broken incompatibly), we're moving most code over to |
@@ -11,5 +11,4 @@ requires "nimcrypto >= 0.5.4 & < 0.6.0" | |||
requires "ngtcp2 >= 0.32.0 & < 0.33.0" | |||
requires "upraises >= 0.1.0 & < 0.2.0" |
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 is another dep we don't use in low-level libs
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.
For upraises, I would argue we won't need it in a few months anyway (after 1.6 migration), so it might be easier to just remove it then
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.
it should be a search/replace operation, no? with the new when (NimMajor, NimMinor) < (1, 4): ... else: ...
style, we get rid of the warning and we don't have to use weird/unusual constructs
also, for low-level libs we might want to keep 1.2-compat longer than end-user apps (no point breaking things that work)
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.
It's kinda painful for ie:
https://github.com/status-im/nim-quic/blob/e48bc4547f133ecc8738590d39fd609bdae49337/quic/listener.nim#L31-L33
We didn't have many of thoses in libp2p, but here there are a few such places
@@ -11,5 +11,4 @@ requires "nimcrypto >= 0.5.4 & < 0.6.0" | |||
requires "ngtcp2 >= 0.32.0 & < 0.33.0" | |||
requires "upraises >= 0.1.0 & < 0.2.0" | |||
requires "asynctest >= 0.3.0 & < 0.4.0" |
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.
ditto, this is covered by chronos asyncTest
export datagram | ||
export errors | ||
export upraises | ||
|
||
template getOr*[T](o: Option[T], otherwise: untyped): T = |
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 is valueOr
in stew/results
or "why Questionable is awesome"
ref vacp2p/nim-libp2p#725 (comment)
I tried to introduce a light version of Questionable, but just handling
without hello =? someOption
is already quite heavy (macros, templates, procs..)So instead I made my own bad version :)