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

define keys on RegexMatch #37299

Merged
merged 1 commit into from
Jan 21, 2021
Merged

Conversation

oxinabox
Copy link
Contributor

@oxinabox oxinabox commented Aug 31, 2020

This came up in discussion in #34355
Seperating it out from that PR because it is a far less clear question/answer.

In general if we define getindex we should define keys.

Some things in this PR you might take issue with:

### 1. It returns a tuple. `keys` only promises that it will return a iterator. `keys` on a `NamedTuple` returns a `Tuple`, `keys` on a `Vector` returns a `OneTo`. A `Tuple` just seems like the simplest thing to do this will normally be small so i didn't think some kind of `KeySet` would be appropriate also the keys *are* infact ordered, just like the are for a `NamedTuple`.

2. It returns a mix of named and number keys.

We can't return all named since they might not all be named.
We could return all numbered, I would be down with that.
Very easy PR. keys(m::RegexMatch = keys(m.captures).

It seems like this is what we want since this is what show currently displays.

@oxinabox
Copy link
Contributor Author

oxinabox commented Aug 31, 2020

3. in consistent with haskey

If combined with #37300 we do end up with a similar problem to what we currently have for NamedTuples where haskey and keys disagree with each other.
haskey(nt, 1)==true but 1 in keys(nt)==false (if the first capture group was named)

One way to resolve this would be with a special object that overloads in to accept both.
Should that be done?
Doing that would make us less consistent with NamedTuples however.

@KristofferC
Copy link
Member

KristofferC commented Aug 31, 2020

Tuple just seems like the simplest thing to do this will normally be small so i didn't think some kind of KeySet would be appropriate

Is it really simpler than e.g. a Vector{Union{Int, Symbol}}? If you have a function that you send these keys into, it will specialize for all permutations of Symbol and Int you pass in, which is perhaps not what you want.

@oxinabox
Copy link
Contributor Author

Is it really simpler than e.g. a Vector{Union{Int, Symbol}}?

No it isn't.
I meant that it is simpler than defining a new KeySet like type.
Definately could change to returning a Vector.

@KristofferC
Copy link
Member

I meant that it is simpler than defining a new KeySet like type.

I guess the advantage is that with a KeySet like type you don't need to allocate?

@StefanKarpinski StefanKarpinski added the triage This should be discussed on a triage call label Sep 11, 2020
@JeffBezanson
Copy link
Member

This is a good feature, but I definitely think it should not return a tuple. A Vector would be fine. The tuple version is type-unstable, and I doubt you would ever want code specialized on this.

base/regex.jl Outdated Show resolved Hide resolved
test/regex.jl Outdated Show resolved Hide resolved
@JeffBezanson
Copy link
Member

Please fix test and rebase.

test/regex.jl Outdated Show resolved Hide resolved
@oxinabox
Copy link
Contributor Author

oxinabox commented Jan 2, 2021

I thought this was merged ages ago.
Can we merge it so it stops accumulating conflicts?

@oxinabox
Copy link
Contributor Author

Bump

add to News.md about defining keys on RegexMatch

fix whitespace

return a Vector from keys(::RegexMatch)

correct test of RegexMatch keys

regex keys displayed are strings (since that is how we get them from PCRE)
@oxinabox
Copy link
Contributor Author

oxinabox commented Jan 19, 2021

Bump.
As far as I know noone has voiced any objection to this feature in the 6 months this PR has been open.
Other than using a Vector rather than a tuple which has beeb resolved also for about 6 months.
Is there anything at all i need to do or see resolved?

@JeffBezanson JeffBezanson merged commit 380abc8 into JuliaLang:master Jan 21, 2021
@oxinabox oxinabox deleted the ox/regexkeys branch January 21, 2021 17:27
@vtjnash vtjnash removed the triage This should be discussed on a triage call label Apr 8, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
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.

6 participants