GoogleChromeLabs / simplehttp2server

A simple HTTP/2 server for development
Other
1.74k stars 99 forks source link

extglob.go #36

Open ncruces opened 6 years ago

ncruces commented 6 years ago

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 commented 6 years ago

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

ncruces commented 6 years ago

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:

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

OK, sure!

ncruces commented 6 years ago

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.