search

nitely / nim-regex

Pure Nim regex engine. Guarantees linear time matching
https://nitely.github.io/nim-regex/
MIT License
227 stars 21 forks source link

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

Closed honewatson closed 6 years ago

nitely commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

honewatson commented 6 years ago

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

nitely commented 6 years ago

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 commented 6 years ago

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!