Change zstdless behavior to align with zless#2909
Conversation
felixhandte
left a comment
felixhandte
left a comment
There was a problem hiding this comment.
This definitely looks like the right approach! I have one nit.
Maybe this is outside the scope of this PR, but it makes me uncomfortable that we have no tests for zstdless. I see we have at least a couple for zstdgrep and zstdcat in playTest.sh. Maybe we could add a couple for zstdless?
|
Ok, looks good in terms of applying the requested changes. Did we decide adding tests is outside the scope of this PR? It would be nice to at least have a trivial few. |
|
We don't need complex tests, but basic ones would be welcome. That being said, I don't think they should be part of Probably making a new small |
@Cyan4973 What scenarios are you thinking of? We do already use |
Therefore, the comment I made regarding I presume these tests are currently present in Refactoring tests for |
|
Added some very basic tests, I will expand on these and move both zstdgrep and zstdless tests in a followup. |
|
Updated and also added environment variable to specify zstd path, necessary for some of the testing platforms |
felixhandte
left a comment
felixhandte
left a comment
There was a problem hiding this comment.
Cool. A couple nits. I guess you'd said you were planning to follow up with more thorough tests? The major thing that seems to be missing is passing flags to zstdless and seeing that they go to less and not zstd.
felixhandte
left a comment
felixhandte
left a comment
There was a problem hiding this comment.
Oops, looks like Circle CI isn't happy:
/home/circleci/project/tests/../programs/zstdless: 8: exec: less: not found
I guess it's reasonable to wrap all the tests in an
if [ -n "$(which less)"]; then
...
fi
|
Test issues fixed |
…ter_refactor * origin/dev: AsyncIO compression part 1 - refactor of existing asyncio code (facebook#3021) cleanup double word in comment. slightly shortened status and summary lines in very verbose mode Change zstdless behavior to align with zless (facebook#2909) slightly shortened compression status update line More descriptive exclusion error; updated docs and copyright Typo (and missing commit) Suggestion from code review Python style change Fixed bugs found in other projects Updated README Test and tidy Feature parity with original shell script; needs further testing Work-in-progress; annotated types, added docs, parsed and resolved excluded files Using faster Python script to amalgamate
5 participants
Flags for zstdless will now be passed to less instead of zstd.
Not for immediate merge; this will change the behavior for zstdless and would be better to bake in dev for awhile rather than going into the impending release.