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

Add --files-from and --files-from0 options #321

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rotty
Copy link

@rotty rotty commented Jun 18, 2015

This makes attic usable with custom file listers (e.g. 'find'
invocations) and pre-assembled file lists. Both
newline-delimited (--files-from) and NUL-delimited (--files-from0)
formats are supported.

This makes attic usable with custom file listers (e.g. 'find'
invocations) and pre-assembled file lists. Both
newline-delimited (--files-from) and NUL-delimited (--files-from0)
formats are supported.
@ThomasWaldmann
Copy link
Contributor

could you add some unit tests?

@rotty
Copy link
Author

rotty commented Jun 18, 2015

I'll do so, hopefully in this weekend.

Test the basic functionality of "attic create" with --files-from and
--files-from0 options. Note that the tests are not very exhaustive;
missing in particular is:

- reading from stdin, as the test harness doesn't emulate
  sys.stdin.buffer.
- use of multiple --files-from and/or --files-from0 options.
@rotty
Copy link
Author

rotty commented Jun 28, 2015

I've now added basic unit tests, however, as noted in the commit message, these are not yet very comprehensive. If you decide to merge this branch at this point nevertheless, feel free to squash the two commits (code and unit test).

def __call__(self, string):
result = super().__call__(string)
# Work around http://bugs.python.org/issue14156
if self._binary and result is sys.stdin or result is sys.stdout:
Copy link
Contributor

Choose a reason for hiding this comment

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

is the line above doing what you want?

Copy link
Author

Choose a reason for hiding this comment

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

Manual tests using stdin work (this is my primary use case, after all), however, to add automatic tests, one would need to extend 'ArchiverTestCaseBase.attic()' to emulate sys.stdin.buffer to enable binary input from stdin.

Copy link
Contributor

Choose a reason for hiding this comment

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

do you mean it like this: if (self._binary and result is sys.stdin) or result is sys.stdout: ?

Copy link
Author

Choose a reason for hiding this comment

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

TW notifications@github.com writes:

+class FileType(argparse.FileType):
+

  • [...]
  • def call(self, string):
  •    result = super().**call**(string)
    
  •    # Work around http://bugs.python.org/issue14156
    
  •    if self._binary and result is sys.stdin or result is sys.stdout:
    

do you mean it like this: if (self._binary and result is sys.stdin)
or result is sys.stdout: ?

Oh, good catch, there is a set of parens missing (should be 'if
self._binary and (result is sys.stdin or result is
sys.stdout)'. Obviously, I didn't test with sys.stdout. I'll fix this,
along with adding a unit test for this class.

Thanks, Rotty

Copy link
Author

Choose a reason for hiding this comment

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

Another thing that occurred to me, and which i'm not exactly sure what to do about, is that the code in FileType.__call__() monkey-patches (i.e., adds attributes to) sys.stdin or sys.stderr, if they are the result of super().__call__().

I think this monkey-patching is fine for newly-created file objects, but less so for sys.stdin and sys.stdout. It might however be acceptable in the context of an application (as opposed to a general-purpose library). Do you have an opinion on that topic?

Before this change, the FileType wrapper would always return a binary
file object when used with stdout, regardless of the file mode.
@rotty
Copy link
Author

rotty commented Jul 5, 2015

Oh, that Travis CI failure is sneaky: I can reproduce it locally, but it only happens when running python3 -m attic.testsuite.run -b, but not without -b ("Buffer stdout and stderr during tests").

Whem the test suite is run with the '-b' option, the tests methods no
longer have sys.stdout.buffer available, as sys.stdout is replaced by a
StringIO instance.
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

Successfully merging this pull request may close these issues.

2 participants