-
Notifications
You must be signed in to change notification settings - Fork 368
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
Contributing named capture groups to pathToRegexp #305
Comments
I think the answer is no, it's not particularly something I'd want to maintain in the long run. Is there a good pro argument as to why it's an improvement? Keep in mind named capture groups aren't supported everywhere either. I’d rather not maintain something “just because we can”. |
Is support for old IE versions important for this project? And what kind of maintenance burden would this impose? Sorry if these are dumb questions, I'm new to this OSS thing. |
Not a large one, but unfortunately any breaking change would be a new major version. Behind a flag removes that concern though, since it'd be opt-in. This module is used in browsers so maximum browsers support is preferred.
Mostly ensuring I keep it supported and documentation up-to-date. Since I maintain a large number of packages and have limited time, I prefer to avoid adding things that don't seem to add much value. In this case, I worry that at best it does nothing except add package weight (which is also a concern, re browsers) and at worst it ends up creating new issues or bugs (because So given these concerns, I like to know in an issue why it'd be a useful addition. Any extra code grows the package (bad for browsers, maintenance, and documentation). So sadly I avoid doing things nowadays just because I can. Side note: Another place of concern would be supported characters for named capturing groups. I'm not familiar with what the character ranges are, but they might deviate from what this package supports. If not today, then in a new release. For example, |
Valid names:
Package weight:
Is even a little new code a major concern? Naively the whole "feature" would be like 5 lines of changed code. Why:
TBH I have no specific use case. It's useful insofar named groups existing in regexes is useful. Because it seemed like a simple change, I thought it would be a good opportunity for a first try in contributing to an open source project. |
Ultimately my case would be:
But correct me if any of my assumptions are wrong. |
As a node-only package, not really. As a browser package, I do try to consider it. If I thought it'd be used often it's a no brainer, but so far only really one person has asked and Line 49 in 80a1eb6
As a general reference, I would prefer to tunnel people away from the raw regex and into using FWIW, I think you are completely correct with all this, I just haven't seen anyone needing the feature enough to need the extra lines of code. I understand that's unfortunate as a first contribution and I'm sorry about that. I do want to just say yes to you anyway but I think it'd be much better to spend your time on a different issue instead. If that's what you're looking for, I can think about a different feature or package that might make a good first contribution. Are you specifically interested in this package or any other particular type of feature you'd want to contribute? |
Ah, that is unfortunate. As for what led me here, I happened to look at the code because it threw an error when I tried to name my own group and I looked around the issues for any reason why named groups were not in already in and I found that post where you said you're open to a PR and because it seemed/seems such an easy change, I decided it was a good first try. I can't really think of any other feature to help with - my use case for this package is really simple to begin with, so I haven't ran into anything special that needs doing. |
Can you elaborate on this? What errored or what were you trying to do? |
Ah, well, "named parameters" aren't really named in any way in the context of So I kinda tried something funny - I tried to name what the docs call "Unnamed parameters" (the irony is delightful). And then you get the error |
Hello everyone!
I would like to add an option to the
pathToRegexp
to capture named groups. I searched for previous issues, as this is likely to have come up before - named groups seem a natural match to the already existing named parameters - and indeed there are many.The question is: Would you still be open to accepting a PR to that effect as noted here: #240 (comment) or has sentiment changed?
The text was updated successfully, but these errors were encountered: