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

extglob.go #36

Open
ncruces opened this issue Feb 3, 2018 · 7 comments
Open

extglob.go #36

ncruces opened this issue Feb 3, 2018 · 7 comments

Comments

@ncruces
Copy link
Contributor

ncruces commented Feb 3, 2018

Hi,

I've redone extglob.go from scratch, lifted a bunch of tests from micromatch.

Only major inaccuracy left (vs. bash), is handling of dotglob (mine/yours matches hidden files by default).

Are you guys interested? If so, I can either contribute, or spin this into its own package.
Haven't done so yet because this is really my first golang effort.

Also planning to implement captures next.

@surma
Copy link
Contributor

surma commented Feb 3, 2018

Firebase uses extglob for node, so I am mostly interesting in compatibility in that package. Happy to accept an PR :)

@ncruces
Copy link
Contributor Author

ncruces commented Feb 3, 2018

That's actually the library I used for test cases. I mean, extglob is the bit that implements patterns like @(pattern-list), you need micromatch for the rest.

I'm not claiming 100% compatibility, but it's certainly improved. Remaining differences are:

  • no support for !(pattern-list) (can't be done trivially with Go regexp); and
  • handling of dotglob.

Most other tests should pass (even if I didn't translate them).

I'm not using simplehttp2server myself. I could do PR with my version of these two files, which are API compatible with yours, but our implementations would diverge in the future? Or I could spin this into a package and do a PR using that package, but then you'd lose control over updates?

What do you think? And bear with me this is my first Go project, I'm not even sure how to best do a package. Happy to learn, though.

@surma
Copy link
Contributor

surma commented Feb 3, 2018

What do you think? And bear with me this is my first Go project, I'm not even sure how to best do a package. Happy to learn, though.

Ooooh cool! Congrats! Also a good choice for a first project.

How about this: You PR your version into my repo (do you have tests?) and I’ll give you a proper code review for functionality and Go idioms. Our packages diverging later is fine, I can always remove the inlined version and depend on your package at that point.

@ncruces
Copy link
Contributor Author

ncruces commented Feb 13, 2018

Have you had a chance to review this? I've implemented capture redirects in my version (which covers all the features documented by Firebase, if not all the features supported by it), but I'd like to have some feedback before pushing that one.

@surma
Copy link
Contributor

surma commented Feb 13, 2018

Sorry for the delay! Last 2 weeks have been a bit crazy with travel and conferences. I’ll get back to this soon, please give me some more time :)

@ncruces
Copy link
Contributor Author

ncruces commented Feb 13, 2018

OK, sure!

@ncruces
Copy link
Contributor Author

ncruces commented Feb 25, 2018

Thanks! When I have more time, I'll do a PR for the capture redirects feature.

My version is here. The general idea is to transform these into named capturing groups of the regex, and whenever there are capturing groups, compile a template for the destination and do a ReplaceAllString.

I need to improve unit testing of these.

I'm also planning to add a feature that is not documented, but I believe works on Firebase as well, which is negative matches. Not negative sub-matches (those are AFAIK impossible to implement with Go regex), but negating an entire match (so match everything but this) which is often useful and easy.

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

No branches or pull requests

2 participants