nim-lang / RFCs

A repository for your Nim proposals.
135 stars 26 forks source link

deprecate API's with string/seq + start/last in favor of ones with openArray #381

Open timotheecour opened 3 years ago

timotheecour commented 3 years ago

proposal

benefits

example 1

refs https://github.com/nim-lang/Nim/pull/18028#issuecomment-841790728

example 2

(refs https://github.com/nim-lang/Nim/pull/18173) optional start/last parameters lead to bad API's where a valid value ends up hijacked with a different meaning, eg:

func find*(a: SkipTable, s, sub: string, start: Natural = 0, last = 0): int

here, when user specifies last = 0, it ends up meaning last = sub.high, so there's no way for user to specify the edge condition of last = 0, which should be a valid choice

changing the signature to func find*(a: SkipTable, s, sub: string, start: Natural = 0, last = -1): int would be an improvement but is still unsatisfactory, as this could equally be interpreted as meaning s.high or empty slice of s (EDIT: func find*(a: SkipTable, s, sub: string, start: Natural = 0, last = sub.high): int would be possible too)

instead, func find*(a: SkipTable, s: openArray[char], sub: string): int would be self-documenting and not have any edge cases, the user then would call simply toOpenArray(s, start, end) if they want a slice

Araq commented 3 years ago

here, when user specifies last = 0, it ends up meaning last = sub.high, so there's no way for user to specify the edge condition of last = 0, which should be a valid choice

Well but Nim allows for

func find*(a: SkipTable, s, sub: string, start: Natural = 0, last = sub.high): int

it's just that the stdlib was written before this feature existed (and I personally don't like the feature).

It's a minor point, but RFCs should be correct. :-)

Regarding the RFC itself, IMO we can simply write new code with openArray and leave the old code untouched, I don't enjoy deprecating stuff that works well. Yes really, it does work well -- you can use Nim as a better Basic thanks to strutils and openArray soon enough will bite back with the required ownership rules.

timotheecour commented 3 years ago

It's a minor point, but RFCs should be correct. :-)

yes, I've updated RFC and included that sub.high as alternative

leave the old code untouched, I don't enjoy deprecating stuff that works well.

except that old code tends to not use sub.high and falls into "example 2" category, causing edge conditions for otherwise valid params (eg https://github.com/nim-lang/Nim/pull/18173, there are/were other similar cases). Also uniformity in APIs (when applied to best practices) is still a good thing.

So we can still introduce the openArray overload for old APIs (with the special care described in https://github.com/nim-lang/Nim/pull/18173#issuecomment-855012363 to ensure the openArray overload is selected when stard/end is unspecified), and whether to deprecate/soft-deprecate (with just a doc comment)/no-deprecate is secondary, at least new code can use old APIs using new style.

soon enough will bite back with the required ownership rules

do you have an example?

Araq commented 3 years ago

do you have an example?

You cannot write proc echo(x: varargs[openArray[char]]). Now you can argue that this should be allowed, as you can for all the other cases I could come up with, but this means we need to refine first-class openArray support first, make it non-experimental etc -- before we can build new APIs on top of it.