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

allow convert from RegexMatch to Dict/NamedTuple #50988

Merged
merged 13 commits into from Mar 4, 2024
Merged

allow convert from RegexMatch to Dict/NamedTuple #50988

merged 13 commits into from Mar 4, 2024

Conversation

aplavin
Copy link
Contributor

@aplavin aplavin commented Aug 20, 2023

Added implementation + tests, will add to docs if ok

@brenhinkeller brenhinkeller added the feature Indicates new feature / enhancement requests label Aug 21, 2023
@aplavin
Copy link
Contributor Author

aplavin commented Sep 16, 2023

bump

@DilumAluthge DilumAluthge added needs docs Documentation for this change is required needs news A NEWS entry is required for this change triage This should be discussed on a triage call labels Sep 16, 2023
@aplavin
Copy link
Contributor Author

aplavin commented Sep 29, 2023

bump...

@DilumAluthge
Copy link
Member

Let's see if we can find a reviewer for this.

@DilumAluthge DilumAluthge added the strings "Strings!" label Sep 29, 2023
vtjnash
vtjnash reviewed Sep 30, 2023
View reviewed changes
base/regex.jl Show resolved Hide resolved
vtjnash
vtjnash reviewed Sep 30, 2023
View reviewed changes
base/regex.jl Outdated Show resolved Hide resolved
vtjnash
vtjnash reviewed Sep 30, 2023
View reviewed changes
base/regex.jl Outdated Show resolved Hide resolved
@aplavin
Copy link
Contributor Author

aplavin commented Nov 26, 2023

Bump...
Very often when working with regexes I wish I could easily convert the match to a namedtuple, and operate on it with a wide range of familiar tools.

@aplavin
Copy link
Contributor Author

aplavin commented Dec 27, 2023

bump...

@StefanKarpinski
Copy link
Sponsor Member

Let's discuss on the next triage call. cc @LilithHafner

@StefanKarpinski StefanKarpinski changed the title convert RegexMatch to Dict and NamedTuple allow convert from RegexMatch to Dict/NamedTuple Jan 18, 2024
@LilithHafner
Copy link
Member

triage is cool with this. (both NamedTuple and Dict) The fact that the dual indexing lines up is a nice factor.

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Jan 18, 2024 •
edited

Triage approves. (Confusing title was causing much delay unfortunately; have edited for clarity: this PR allows converting RegexMatch objects to NamedTuples and Dicts.) While it's true that converting to NamedTuple goes from value domain to type domain, it was observed that very often the regex object is a compile-time constant and it would actually be possible to optimize the NamedTuple construction to specialize on the names specified in the regex string macro call. @ericphanson pointed out that this might require capturing names in the RegexMatch object, but I'm not entirely sure if that's necessary with constant propagation. The point is not that we even need that optimization, but that it is in principle possible, which weakens the performance argument: converting from statically known regex to NamedTuple is not inherently slow, it just happens to be slow. Conversion to Dict seems uncontroversial.

@LilithHafner LilithHafner removed the triage This should be discussed on a triage call label Jan 18, 2024
@LilithHafner
Copy link
Member

Still needs docs and news, then should be good to merge!

JeffBezanson
JeffBezanson reviewed Feb 7, 2024
View reviewed changes
NEWS.md Outdated Show resolved Hide resolved
JeffBezanson
JeffBezanson reviewed Feb 7, 2024
View reviewed changes
base/regex.jl Outdated Show resolved Hide resolved
@aplavin @JeffBezanson
Update NEWS.md
78b53ca 
Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
JeffBezanson
JeffBezanson reviewed Feb 7, 2024
View reviewed changes
base/regex.jl Outdated Show resolved Hide resolved
@aplavin @JeffBezanson
Update base/regex.jl
83c2584 
Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
JeffBezanson
JeffBezanson reviewed Feb 7, 2024
View reviewed changes
base/regex.jl Outdated Show resolved Hide resolved
@aplavin @JeffBezanson
Update base/regex.jl
ef288fa 
Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
JeffBezanson
JeffBezanson reviewed Feb 7, 2024
View reviewed changes
base/regex.jl Outdated Show resolved Hide resolved
@aplavin @JeffBezanson
Update base/regex.jl
d155a2d 
Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
JeffBezanson
JeffBezanson reviewed Feb 7, 2024
View reviewed changes
base/regex.jl Outdated Show resolved Hide resolved
aplavin and others added 2 commits February 7, 2024 17:49
@aplavin @JeffBezanson
Update base/regex.jl
c279c27 
Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
@aplavin @JeffBezanson
Update base/regex.jl
3810389 
Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
@JeffBezanson
Copy link
Sponsor Member

Note we should not use "convert" terminology, to avoid confusion with convert.

@JeffBezanson
Copy link
Sponsor Member

Bump. This is ready to go, just needs a rebase.

@oscardssmith oscardssmith added the merge me PR is reviewed. Merge when all tests are passing label Feb 27, 2024
@IanButterworth IanButterworth removed needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Feb 29, 2024
@IanButterworth
Copy link
Sponsor Member

IanButterworth commented Mar 3, 2024 •
edited

The conflict appears to be minor in NEWS.md

@aplavin
Copy link
Contributor Author

aplavin commented Mar 3, 2024 •
edited

I wasn't really sure if this step is something I need to do, or it is resolved almost automatically by the person merging the PR. Clicked resolve now, sorry if this misunderstanding led to a delay.

@IanButterworth
Copy link
Sponsor Member

It can be done by people with commit rights but requires the "maintainers can edit" option to be enabled, which I don't think it was for this

@IanButterworth IanButterworth merged commit bc2212c into JuliaLang:master Mar 4, 2024
7 checks passed
@IanButterworth IanButterworth removed the merge me PR is reviewed. Merge when all tests are passing label Mar 4, 2024
tecosaur pushed a commit to tecosaur/julia that referenced this pull request Mar 4, 2024
@aplavin @DilumAluthge
@JeffBezanson @tecosaur
allow convert from RegexMatch to Dict/NamedTuple (JuliaLang#50988)
4c969d2 
Added implementation + tests, will add to docs if ok

---------

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
mkitti pushed a commit to mkitti/julia that referenced this pull request Apr 13, 2024
@aplavin @DilumAluthge
@JeffBezanson @mkitti
allow convert from RegexMatch to Dict/NamedTuple (JuliaLang#50988)
30a00db 
Added implementation + tests, will add to docs if ok

---------

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

@JeffBezanson JeffBezanson JeffBezanson left review comments

@StefanKarpinski StefanKarpinski Awaiting requested review from StefanKarpinski

Assignees
No one assigned
Labels
feature Indicates new feature / enhancement requests strings "Strings!"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@aplavin @DilumAluthge @StefanKarpinski @LilithHafner @JeffBezanson @IanButterworth @vtjnash @brenhinkeller @oscardssmith