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

merge file objects using references (not copy.copy) #437

Merged
merged 1 commit into from Jan 29, 2018

Conversation

Projects
None yet
2 participants
@lmiphay
Contributor

lmiphay commented Apr 11, 2017

copy.copy "...does not copy types like ..., file, ..., or any similar types"

file objects (e.g. err_stream... etc) end up closed following a dict merge.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Apr 12, 2017

Huh, was totally unaware of that, thanks for the catch!

@bitprophet bitprophet added this to the 0.15.x milestone Apr 12, 2017

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jan 29, 2018

An interesting wrinkle: we must never have actually tested what happens when users put true file objects in their config, because under Python 3, copy.copy explodes like this:

Traceback (most recent call last):
  File "/usr/local/var/pyenv/versions/3.5.2/lib/python3.5/unittest/case.py", line 58, in testPartExecutor
    yield
  File "/usr/local/var/pyenv/versions/3.5.2/lib/python3.5/unittest/case.py", line 600, in run
    testMethod()
  File "/Users/jforcier/.virtualenvs/invoke3/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/Users/jforcier/Code/oss/invoke/tests/merge_dicts.py", line 91, in merge_file_types_by_reference
    merge_dicts(d1, d2)
  File "/Users/jforcier/Code/oss/invoke/invoke/config.py", line 1177, in merge_dicts
    base[key] = copy.copy(value)
  File "/Users/jforcier/.virtualenvs/invoke3/lib/python3.5/copy.py", line 97, in copy
    rv = reductor(4)
TypeError: cannot serialize '_io.TextIOWrapper' object

Complicating things is that Python 3 dropped file entirely in favor of io.* classes, and six doesn't appear to have a convenient wrapper for testing "is this a file object", so I guess we have to write our own stupid little shim here. (Since duck typing isn't going to be sufficient in this case - e.g. plenty of other classes might define open or close and not have problems with copy.copy.)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jan 29, 2018

Hm, thinking we may be able to get away by using invoke.util.has_fileno - non-fileno-bearing FLOs like io.StringIO seem to copy.copy no problem, and IIRC all "real" file objects will pass this particular test.

Given that right now, file objects are simply not useful at all in the config machinery, even if this isn't perfect it's still a big step up. Should users find more edge cases here we can then fall back to something like try: if isinstance(x, file) / except NameError: if isinstance(x, io.IOBase) or whatever.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jan 29, 2018

Not quite, has_fileno actually calls fileno which complicates things. It's intended to catch things that look like files but don't map to real files on the fileystem. In this case here, I suspect we'd still be OK if we just used the simpler hasattr('fileno') (i.e....maybe that is some good duck typing we can use.)

@bitprophet bitprophet merged commit 6167a8c into pyinvoke:master Jan 29, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details

bitprophet added a commit that referenced this pull request Jan 29, 2018

bitprophet added a commit that referenced this pull request Jan 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment