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

"option -p: Could not parse pattern" when the pattern contains a minus sign in it #220

Closed
ozgurakgun opened this issue May 6, 2018 · 17 comments

Comments

@ozgurakgun
Copy link
Contributor

This used to work before the new pattern language of 1.0: -p type-checking. Now it gives the following error message: option -p: Could not parse pattern.

Is there a suggested way of rewriting this pattern, or should I rename my tests to not include minus signs in them?

Thanks!

@UnkindPartition
Copy link
Owner

Try -p /type-checking/. Here's the relevant part of the docs:

As an extension to the awk expression language, if a pattern pat contains only
letters, digits, and characters from the set [_/ ], it is treated like /pat/
(and therefore matched against $0). This is so that we can use -p foo as
a shortcut for -p /foo/.

@UnkindPartition
Copy link
Owner

Perhaps - should be added to the list of safe characters, given that it can be part of normal words like type-checking.

@ozgurakgun
Copy link
Contributor Author

Thanks for the very quick response, I had read the docs but somehow missed that part. I am happy enough with the forward slashes.

@ozgurakgun
Copy link
Contributor Author

Oh, I spoke too early. When I try another pattern with forward slashes, the meaning seems to change. The docs say "-p foo is a shortcut for -p /foo/" but they don't seem to do the same thing if the pattern itself contains a forward slash.

In my example, -p issues/390 works but -p /issues/390/ doesn't.

So if there is both a forward slash and a minus sign in a pattern there is no way of passing it in verbatim, as far as I can see?

Alternatively, is there a way to escape the minus sign for the pattern in the original post?

@UnkindPartition
Copy link
Owner

Hmm, yeah, I shouldn't include / in the set of safe characters, because it obviously conflicts with the /.../ syntax.

The correct pattern (after I fix this bug) should be -p /issues\/390/, where the inner / is escaped.

@UnkindPartition
Copy link
Owner

UnkindPartition commented May 6, 2018

I think my original reasoning for including / was like this:

If you only use / but no other special AWK characters, then you don't need the /.../ syntax.

But it's better to be consistent with /.../ and make it always work.

@UnkindPartition
Copy link
Owner

I removed / and added - to the safe character list in master — could you check if that works for you?

@ozgurakgun
Copy link
Contributor Author

I am not sure how to depend on the repository version using stack, sorry. If the two examples I gave above (type-checking and issues/390) work, then I am happy.

@UnkindPartition
Copy link
Owner

I am not sure how to depend on the repository version using stack, sorry.

See https://docs.haskellstack.org/en/stable/yaml_configuration/#git-and-mercurial-repos

If the two examples I gave above (type-checking and issues/390) work, then I am happy.

type-checking now works both as -p /type-checking/ and -p type-checking. issues/390 only works as /issues\/390/ because / is a special symbol.

@ozgurakgun
Copy link
Contributor Author

Oh I didn't know the subdirs field, that works now.

-p type-checking indeed works, and I seem to need to pass -p '/issues\/390/' to make issues/390 work.

Are you sure about this change? Because I assume people would be using the likes of testGroup to group tests and would depend on this behaviour staying somewhat backwards compatible? In my example here "issues" is a test group and 390 is an individual test.

@UnkindPartition
Copy link
Owner

Oh yeah, I forgot about / as a field separator. That does seem a bit unfortunate.

An alternative would be to change the field separator to something else, like .. Then . does not conflict with the /.../ syntax and can also be made safe. What do you think?

@ozgurakgun
Copy link
Contributor Author

Personally I would be wary of breaking the existing /-patterns.

What's wrong with allowing both - and / in raw patterns?

@UnkindPartition
Copy link
Owner

What's wrong with allowing both - and / in raw patterns?

As you yourself pointed out,

In my example, -p issues/390 works but -p /issues/390/ doesn't.

This is because the surrounding slashes in -p /issues/390/ are not interpreted as the /.../ syntax but as literal slashes.

@ozgurakgun
Copy link
Contributor Author

Of course, you are right.

Would requiring characters like - to be escaped (maybe with a ) be an option?

Either way, now that I think about this again, using / to mean two different things (field separator and the /.../ syntax) seems like asking for trouble/confusion. A different field separator (despite being backwards incompatible) may help as you suggest.

@UnkindPartition
Copy link
Owner

Would requiring characters like - to be escaped (maybe with a ) be an option?

The interpretation of - has nothing to do with this; this is purely about how we interpret / and what the separator is.

@UnkindPartition
Copy link
Owner

UnkindPartition commented May 11, 2018

Resolved in tasty 1.1; see the changelog.

@ozgurakgun
Copy link
Contributor Author

Thanks!

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