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

Fix for #9243, nre returns "" instead of nil for missing matches #9755

Merged
merged 2 commits into from
Dec 12, 2018
Merged

Fix for #9243, nre returns "" instead of nil for missing matches #9755

merged 2 commits into from
Dec 12, 2018

Conversation

flaviut
Copy link
Contributor

@flaviut flaviut commented Nov 19, 2018

I'd like community feedback on this PR because it contains some significant breaking nre changes.

@flaviut
Copy link
Contributor Author

flaviut commented Nov 19, 2018

\cc @Araq @timotheecour

@Araq
Copy link
Member

Araq commented Nov 19, 2018

I think this API breakage is fine and unavoidable given that nil is not a valid state for strings anymore.

@timotheecour
Copy link
Member

/cc @flaviut what's the status on this?
also please remember to rebase (not merge) since this is old (if needed using the trick mentioned here:
https://nim-lang.github.io/Nim/contributing.html Commits should be always be rebased against devel (so a fast forward merge can happen))

@flaviut
Copy link
Contributor Author

flaviut commented Dec 11, 2018

@timotheecour it's in good shape, I'd just like to get one other person to review it before I push the button.

@@ -31,6 +31,25 @@

- `options.UnpackError` is no longer a ref type and inherits from `System.Defect` instead of `System.ValueError`.

- nre's `RegexMatch.{captureBounds,captures}[]` no longer return `Option` or
`nil`/`""`, respectivly. Use the newly added `n in p.captures` method to
check if a group is captured, otherwise you'll recieve an exception.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haven't thought about it much but is it possible to get a CT (instead of RT) error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not unless the halting problem is solved. It's unknown whether a certain variable corresponds to a certain pattern. With constants it might be possible, but only when a group isn't optional (i.e. (foo)? will never fail at CT), and only if nim starts supporting compile-time FFI.

Copy link
Member

@timotheecour timotheecour Dec 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @flaviut
that's not what I meant, but I worded it poorly; see https://github.com/nim-lang/Nim/issues/9941

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants