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.

Sign up for GitHub

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

Jump to bottom

Add nre to stdlib #2818

Merged
merged 156 commits into from
Jun 11, 2015
Merged

Add nre to stdlib #2818

merged 156 commits into from
Jun 11, 2015

Conversation

flaviut
Copy link
Contributor

@flaviut flaviut commented May 26, 2015

Fixes #2511

@flaviut
Initial Commit
b22a3e8
@flaviut
Add thin PCRE wrapper
b36ba63
@flaviut
Implement regex initialization
dec2f9e
@flaviut
Add unit tests
685e4a1
@flaviut
Fix typos
3a6e794
@flaviut
Extend Regex, add RegexMatch
c4d536f
@flaviut
Remove DUPNAMES option
bfba4b9 
It makes the rest of the implementation more complicated and is confusing
anyway. The user expects a 1:1 mapping between a match and a name
@flaviut
Implement foundation for named captures
00b047a
@flaviut
Implement correct destruction
ca4cf24
@flaviut
Change test execution technique
be984d3
@flaviut
Simplify thin wrapper
613d12c
@flaviut
Implement regex execution
2db1a54
@flaviut
Implement captures
721ea11
@flaviut
captureCount doesn't need to be exposed that way
d2c20a3
@flaviut
Add access to capture count and names
48c29ac
@flaviut
Move some things around
0888b84
@flaviut
Remove Option from exec(Regex, string, int)
f692042
@flaviut
Revert "Remove Option from exec(Regex, string, int)"
cc33942 
This reverts commit f692042.
@flaviut
Implement toTable(Captures), toSeq(Captures)
0f4b142
@flaviut
Allow u for unicode matching
ccf5771
@flaviut
Expose pattern and input string
4f4a7df
@flaviut
Rename exec(...) and extend it
d7dbf7e
@flaviut
Expose the pattern string, rename some procs
fb51221
@flaviut
Fix incorrect check
6fe0de0 
pcre_study can return nil and still succeed
The indicator of failure is a non-nil error message
@flaviut
Implement match, find, split
2474758
@flaviut
Flip parameter string-pattern order
c4cb781
@flaviut
Unexpose implementation detail
1652cdb
@flaviut
Add findAllStr
e0cdad6
@flaviut
Added some documentation
efb4f7d
@flaviut
Remove all optional RegexMatch returns
aac71d0
@dom96
Copy link
Contributor

dom96 commented May 27, 2015

👍 for following Nimble conventions on hiding your implementation's private modules :)

@oprypin oprypin mentioned this pull request May 31, 2015
Closed
12 tasks
@dom96
Copy link
Contributor

dom96 commented Jun 4, 2015

Where are we with this?

@Araq
Copy link
Member

Araq commented Jun 5, 2015

@flaviut ping. ready for merging?

@flaviut
Copy link
Contributor Author

flaviut commented Jun 5, 2015

Yep, it is!
On Jun 5, 2015 7:52 AM, "Andreas Rumpf" [email protected] wrote:

@flaviut https://github.com/flaviut ping. ready for merging?

—
Reply to this email directly or view it on GitHub
#2818 (comment).

@Araq
Copy link
Member

Araq commented Jun 6, 2015

Well there is a conflict.

@dom96
Copy link
Contributor

dom96 commented Jun 8, 2015

155 commits? Just for a single new module?

@dom96
Copy link
Contributor

dom96 commented Jun 8, 2015

Wouldn't it be worth to squash these down?

@flaviut
Copy link
Contributor Author

flaviut commented Jun 8, 2015

This contains the complete commit history of nre. That means that people
can use or revert individual commits, along with any other of the other
advantages of source control.
On Jun 7, 2015 8:18 PM, "Dominik Picheta" [email protected] wrote:

155 commits? Just for a single new module?

—
Reply to this email directly or view it on GitHub
#2818 (comment).

@reactormonk
Copy link
Contributor

@dom96 What are more commits gonna cost you? A longer git bisect?

@dom96
Copy link
Contributor

dom96 commented Jun 8, 2015

Alright, fine.

Another thing: wouldn't it make more sense to keep re as it is. Won't rewriting it to use nre create the possibility for breakage?

@oprypin
Copy link
Contributor

oprypin commented Jun 8, 2015

I'd even say breakage is likely. I haven't properly tested that one.

Maybe backwards compatibility is worth more than correctness.

@Araq
Copy link
Member

Araq commented Jun 8, 2015

Well it's weird to deprecate and "fix" re at the same time. I say: Don't touch 're' if it's deprecated anyway.

@flaviut
Revert "Base re off of nre"
3151455 
This reverts commit dc60a51.
@flaviut
Copy link
Contributor Author

flaviut commented Jun 10, 2015

Sorry about the delay, I missed the email notification. I've reverted that commit.

Araq added a commit that referenced this pull request Jun 11, 2015
@Araq
Merge pull request #2818 from flaviut/add-nre
d31fe76 
Add nre to stdlib
@Araq Araq merged commit d31fe76 into nim-lang:devel Jun 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate and replace 're' with 'nre'
5 participants
@flaviut @dom96 @Araq @reactormonk @oprypin