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

Fixes #1 - Allows use of re and fullMatch outside of main module #2

Closed
wants to merge 3 commits into from

Conversation

honewatson
Copy link

No description provided.

@@ -1566,7 +1566,7 @@ iterator peek[T: seq[Rune] | string](s: T): (int, Rune, Rune) {.inline.} =
yield (j, prev, invalidRune)

# todo: make it iterative
proc step(
proc step*(
Copy link
Owner

Choose a reason for hiding this comment

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

are you sure this must be public?

Copy link
Owner

Choose a reason for hiding this comment

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

I just tried this. The issue is in the template stepFrom() below. It seems the method syntax has some limitations and can't be resolved from result.step(...) to step(result, ...), but changing that fixes the issue.

Copy link
Owner

@nitely nitely Jan 14, 2018

Choose a reason for hiding this comment

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

ok, I just fixed it this way ^^

Copy link
Owner

Choose a reason for hiding this comment

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

I reported the issue nim-lang/Nim#7085

@nitely
Copy link
Owner

nitely commented Jan 14, 2018

Hey @honewatson, glad someone is already trying this 😄

There are a ton of test within the module. I was going to split them into tests modules (see the comments named tsome) and avoid using the unittest module in case this ever gets into Nim's stdlib. Of course I'm still working on it, @dom96 made me release it before it was stable 🤣 , so now I have to prioritize things (API stability first).

@nitely
Copy link
Owner

nitely commented Jan 14, 2018

I've fixed most issues here (without exporting non-public APIs). I'd like to keep the test, but not sure if using unittest is a good idea coz of what I commented above. If you can reimplement it with good old doAssert somwhere within the regex module, I'll accept the PR, but if you lack time for it, feel free to close it and I'll do it at some point. Thanks!

@honewatson
Copy link
Author

Not sure how koch testing works but I do think that tests should be independent of the source.

@honewatson
Copy link
Author

Anyway terrific work and well done. Its nice to see it working with the complex email regex out of the box.

@nitely
Copy link
Owner

nitely commented Jan 20, 2018

Not sure how koch testing works but I do think that tests should be independent of the source.

I agree. The tests must be moved to a test folder, like you did here. In fact there are comments the future names of the files (i.e: tescaped_sequences.nim). What I'd prefer is not relying on the unittest module, just in case there is a chance this will make it into stdlib. But I guess we could use unittest and then make changes one day if necessary.

Anyway terrific work and well done. Its nice to see it working with the complex email regex out of the box.

Thanks!! 😄

@nitely
Copy link
Owner

nitely commented Mar 14, 2018

I'm closing this. I've created a tests folder and moved almost all tests there. I'd like to keep the email test from this PR. So if you feel like it, please open a new PR with the test case, but there's no rush to do it. Thanks!

@nitely nitely closed this Mar 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants