Skip to content
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

[nre] captures[i] should return Option[string] instead of recent subtly-breaking change that throws #23

Open
flaviut opened this issue Mar 11, 2019 · 0 comments
Labels

Comments

@flaviut
Copy link
Owner

flaviut commented Mar 11, 2019

From nim-lang/RFCs#109 (comment)

/cc @flaviut
after this recent PR nim-lang/Nim#9755 some of my code broke, but it broke at RT instead of CT, which is not as good as if it failed at CT (because no-one has 100% test coverage, and who knows what other projects would break in the wild...)

how about the following:

* `m.captures[0]` returns the standard `Option[string]` instead of this non-standard `[]` that returns a `string` but throws on un-captured group
  that would've forced an nre user to fix his code and guaranteed it would work 100% ; right now there is no such warrantee until client code hits such an issue in the wild

* furthermore, `Option[string]` leads to cleaner code, eg:
m.captures[0].get("") # case 1: uses options.get with default of `""` for cases where user doesn't care if it's empty vs non-captured, ie matches behavior prior to this PR
m.captures[0].get # case 2: will throw in case of non-captured

where with code after nim-lang/Nim#9755 it's less clean:

if 0 in m.captures: m.captures[0] else: "" # case 1
m.captures[0] # case 2
# note, case 1 could use a helper proc (say, `getOrDefault`) but it'd still not be as clean as if we used `Option`:
m.captures.getOrDefault(0,"")

basically, with Option, things are more composable (and can reuse functionality in std/options); with current API, it's written more awkwardly.

Alternative (for a smoother transition via deprecation path compared to this PR)

keep the old captures (that returns "" instead of nil for no capture) but mark it as deprecated, and add a new capturesOption (or whatever name) that returns an Option[string]

Note

Option[string] would just make everything more consistent, eg with captures.toSeqand captures.toTable that must return seq[Option[string]] anyway /cc @dom96

@flaviut flaviut added the RFC label Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant