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
grep: conflicting flags #1356
grep: conflicting flags #1356
Conversation
exercises/grep/canonical-data.json
Outdated
"files": ["iliad.txt"] | ||
}, | ||
"expected": [ | ||
"2:His wrath pernicious, who ten thousand woes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a desire to match the behavior of grep
? If so, it appears that this output should be iliad.txt
, as I find that -l
takes precedence over -n
in following experiments:
$ grep -n -l hello <(echo hello) (10-08 07:58)
/proc/self/fd/11
$ grep -l -n hello <(echo hello) (10-08 07:58)
/proc/self/fd/11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right. The issue describes the opposite behavior and I didn't check. I will update the test cases accordingly.
exercises/grep/canonical-data.json
Outdated
"description": "One file, several matches, inverted and match entire lines flags", | ||
"property": "grep", | ||
"input": { | ||
"pattern": "Illustrious into Ades premature", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should have a comma inside the string. Otherwise, since the flag to match entire lines and the line in the file is "Illustrious into Ades premature,"
the line will be included in the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, well spotted! I'll fix that.
exercises/grep/canonical-data.json
Outdated
"files": ["iliad.txt", "midsummer-night.txt", "paradise-lost.txt"] | ||
}, | ||
"expected": [ | ||
"2:His wrath pernicious, who ten thousand woes", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if wanting to match the behaviour of grep
, it seems this needs to be ["iliad.txt", "midsummer-night.txt", "paradise-lost.txt"]
, as seen in the following experiment
$ grep -n -l hello <(echo "hello\nworld") <(echo "a\nhello") <(echo "hello\nhello")
/proc/self/fd/11
/proc/self/fd/12
/proc/self/fd/13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as mentioned before I followed the behavior described in the issue which was incorrect. However this should be updated to ["iliad.txt", "paradise-lost.txt"]
since midsummer-night.txt doesn't contain the string "who". I'll update it as well.
exercises/grep/canonical-data.json
Outdated
"description": "Multiple files, several matches, inverted and match entire lines flags", | ||
"property": "grep", | ||
"input": { | ||
"pattern": "Illustrious into Ades premature", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a comma, otherwise it does not match the entire line, as in https://github.com/exercism/problem-specifications/pull/1356/files#r223276145
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll fix that one too.
about precedence of
and
any grep versions with any different behaviours? |
exercises/grep/canonical-data.json
Outdated
@@ -204,6 +204,37 @@ | |||
"files": ["iliad.txt"] | |||
}, | |||
"expected": [] | |||
}, | |||
{ | |||
"description": "One file, one match, line number and file flags", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose that this description should explicitly say which of the two flags takes precedence.
I propose this because it makes the intent of the test easier to understand for a reader of the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes perfect sense. I'll update the test cases with an explicit description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is great, I like these new descriptions!
exercises/grep/canonical-data.json
Outdated
"His wrath pernicious, who ten thousand woes", | ||
"Caused to Achaia's host, sent many a soul", | ||
"And Heroes gave (so stood the will of Jove)", | ||
"To dogs and to all ravening fowls a prey", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing comma at end of this line
exercises/grep/canonical-data.json
Outdated
"files": ["iliad.txt", "midsummer-night.txt", "paradise-lost.txt"] | ||
}, | ||
"expected": [ | ||
"Achilles sing, O Goddess! Peleus' son;", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notice that in the above multiple files
tests, the lines are each prefixed with the file they appear in.
I believe the same should happen here, as demonstrated in an experiment I share below:
$ grep -x -v hello <(echo "hello\nworld") <(echo "a\nhello") <(echo "hello\nhello")
/proc/self/fd/11:world
/proc/self/fd/12:a
Yup, this the new cases are matching |
Sorry about the missing comma, I've made the change in the wrong branch and then forgot to redo it in the correct branch. It should be all good now. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff, and obviously an improvement since it answers some reasonable questions about certain flag combinations.
Noting that this PR added the two specific flag combinations designated in the to-be-closed issue, and the discussion had not turned up any other combinations. I hope that those who see any other interesting combinations open an issue for those.
This PR has been open for almost a full week now. Anyone who has any comments on it should say so within the next 36 hours or so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job @yvincent, thanks!
Thank you @petertseng and @yvincent for addressing this issue! |
resolves #1256.