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

Better regular expressions for CLI tests #387

Merged
merged 1 commit into from
Oct 29, 2016
Merged

Conversation

ndegory
Copy link
Contributor

@ndegory ndegory commented Oct 26, 2016

Refs #370

Rewrite of regexp to be more accurate

How to check:

swarm start
go test ./cmd/amp/cli

has to be rebased after #386 has been merged.

Copy link
Contributor

@subfuzion subfuzion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think overall these look like good improvements. However, I'm not sure that changing the code to use posix character classes -- requiring an extra pair of enclosing brackets -- is worth the effort just for space, and without consensus and guidance, it's an arbitrary change on your part. I'm not opposed, although to me \s is just as readable and a lot less verbose than [[:space:]], and the nuances between perl and posix and non-ASCII support don't really matter here. Of course, once you move beyond \s, '\d, \w classes, [[:alnum:]], [[:alpha:]], [[:blank:]], [[:lower:]], [[:punct:]], [[:upper:]], and [[:xdigit:]] become more readable. On that note, I wonder if we're accepting [[:space:]] in cases where [[:blank:]] is more appropriate? Anyway, we'll move all these into a standard regex dictionary and we can fine tune as needed. Thanks.

@subfuzion
Copy link
Contributor

@ndegory Please resolve conflicts so this can be merged. @bquenin's PR renamed the associated files.

@ndegory ndegory force-pushed the better-cli-test-regexp branch from 8023004 to 78a1bf5 Compare October 28, 2016 07:01
@ndegory
Copy link
Contributor Author

ndegory commented Oct 28, 2016

@subfuzion I'll revert the [[:space:]] to \s if you prefer, anyway it's anecdotal, the main change is that the regexp now are matching different outputs.

@ndegory ndegory force-pushed the better-cli-test-regexp branch from 78a1bf5 to aab3a62 Compare October 28, 2016 07:16
@ndegory ndegory changed the title WIP: better regexp in CLI tests (#370) better regexp in CLI tests (#370) Oct 28, 2016
@subfuzion
Copy link
Contributor

👍

@subfuzion subfuzion merged commit 533ed38 into master Oct 29, 2016
@subfuzion subfuzion deleted the better-cli-test-regexp branch October 29, 2016 00:51
@subfuzion subfuzion added this to the 0.3.0 milestone Oct 29, 2016
@subfuzion subfuzion changed the title better regexp in CLI tests (#370) Better regular expressions for CLI tests Nov 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants