Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement a test covering operation of the cat functionality file. #128
Implement a test covering operation of the cat functionality file. #128
Changes from all commits
ac3295d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
From experience I know that splitting strings and silently assuming a fixed number of resulting parts like this is a bit fragile and tends to break sooner or later. I'd suggest adding a
maxsplit=1
arg to split or usenormname=blabla[0] if additional parts are _always_ irrelevant. Otherwise explicitly assert exactly one occurrence of
'--'` or split and check part count before assignment.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.
On a pedantic note I'm generally not too fond of these one-liner conditionals as they tend to save little and make code less readable IMO. E.g in this case it could easily hide details like if you really meant to test
prefix
to be logicallyTrue
or actually wanted to only target the default value withis not None
. Hereprefix=''
would not really make a difference, but for the general sake it might.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.
Sorry if this comes out blunt after spending a significant amount of time figuring this code bit out, but on a closer look I don't like this
temppath
+_temppath
construct and the out-of-module use of the latter at all. Probably also because I don't really see the point of exposing this part of the existingtemppath
as a new semi-private function with a very similar name and then using it directly in test modules.If the
TEST_OUTPUT_DIR
anchoring should be skipped in some use cases there, that could easily be achieved with, say, askip_output_anchor
argument instead. That would keep all external use to justtemppath
and avoid the naming and functionality confusion as well.I'll see if I can figure that out during merge now that I'm already halfway through.
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.
Minor: private and semi-private functions like these (one or two leading underscores) should generally not be used outside the module itself.
While it's not a blocker they should really either be made fully public (i.e. strip leading underscore and make sure they are generally usable) or the import replaced by existing public functions. The
shared.fileio
already has themakedirs_rec
function, which appears to overlap some 90% percent with_ensuredirs
.I'll take a look and either adjust during merge or add the follow-up marker here.
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.
Minor: as mentioned elsewhere I'd like the WSGI and CGI references in this unit test module to go. From a previous response I understand the origin of this function and that's fine, but delivery method is and should be irrelevant here as we want to (and do) test the underlying functionality backend in general.
I'll probably just generalize the variable name or the function to not even have the
wsgi_variables
arg during merge.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.
Minor: another CGI leftover to strip.
I'll rename to fit unit test module name during merge and add the missing docstring while at it.