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

Allow get/put to use name attribute of StringIO/BytesIO objects in output #699

Merged
merged 6 commits into from Aug 27, 2012

Conversation

Projects
None yet
3 participants
@focusaurus

focusaurus commented Aug 2, 2012

I am using StringIO heavily for files constructed in memory and then passed to put, and wanted log output with a nice meaningful name other than <file obj>.

Here's some sample code to demonstrate this works as before with standard StringIO and BytesIO objects, but if they have a name attribute, use that for log output. Works for both get and put.

@task
def test_put():
    from StringIO import StringIO as S1
    from io import StringIO as S2
    from io import BytesIO
    from fabric.api import put, get
    s1 = S1(u"foo bar")
    put(s1, "/tmp/s1")
    s1.seek(0)
    s1.name = "s2"
    put(s1, "/tmp/s1")

    s2 = S2(u"s2")
    put(s2, "/tmp/s2")
    s2.seek(0)
    s2.name = "s2"
    put(s2, "/tmp/s2")

    s3 = S1()
    get("/tmp/s1", s3)
    s3.seek(0)
    s3.name = "s3"
    get("/tmp/s1", s3)

    s4 = BytesIO()
    get("/tmp/s1", s4)
    s4.seek(0)
    s4.name = "s4"
    get("/tmp/s1", s4)

and here's the output:

[50.57.172.47] Executing task 'test_put'
[50.57.172.47] put: <file obj> -> /tmp/s1
[50.57.172.47] put: s2 -> /tmp/s1
[50.57.172.47] put: <file obj> -> /tmp/s2
[50.57.172.47] put: s2 -> /tmp/s2
[50.57.172.47] download: <file obj> <- /tmp/s1
[50.57.172.47] download: s3 <- /tmp/s1
[50.57.172.47] download: <file obj> <- /tmp/s1
[50.57.172.47] download: s4 <- /tmp/s1

Done.
Disconnecting from 50.57.172.47... done.
@focusaurus

This comment has been minimized.

focusaurus commented Aug 2, 2012

I can add some tests for this if others think this is a worthwhile improvement.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 3, 2012

If this had required a lot of changes I'd be iffy on it due to what I'd perceive as an uncommon (if valid) use case -- but it's a simple change, so I approve. 👍

If you could find a way to add tests (I forget if we got the test ssh server working well for put/get, if it didn't then don't worry about it) and update the documentation, I'll merge this. Thanks!

@focusaurus

This comment has been minimized.

focusaurus commented Aug 3, 2012

OK, cool. Should be able to find some time to ramp up on nose and the fabric test suite sometime next week.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 3, 2012

When you do, it looks like yes we do have tests using file transfer + StringIO, starting here, so that's the type of test to understand & mimic.

@focusaurus

This comment has been minimized.

focusaurus commented Aug 16, 2012

@bitprophet how does that look to you?

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 16, 2012

Looks good in the diff, will check it out locally in a sec.

Protip for the future: when submitting PRs, make a new topic/feature branch, don't dev in master. Makes life a lot easier :)

@focusaurus

This comment has been minimized.

focusaurus commented Aug 16, 2012

Got it. Thanks for the tip.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 16, 2012

Oh, poo. io is Python 2.6+; Fabric has to support 2.5+ for now.

I'm torn between saying we should keep the test & wrap it + the io import within an if sys.version[:2] >= (2, 6), and just saying to drop it as it's not a critical test. I'll leave the decision up to you -- but the suite does need to pass on Python 2.5 in some way.

Thanks again.

@focusaurus

This comment has been minimized.

focusaurus commented Aug 17, 2012

This works on 2.7.2. I don't have a 2.5 handy to test, but unless I made a silly typo it should bypass this test on 2.5.

@travisbot

This comment has been minimized.

travisbot commented Aug 17, 2012

This pull request fails (merged cc8cd2d into e005cb6).

@focusaurus

This comment has been minimized.

focusaurus commented Aug 17, 2012

Hmm...will investigate...

@travisbot

This comment has been minimized.

travisbot commented Aug 17, 2012

This pull request fails (merged 46348d2 into 994a76e).

@focusaurus

This comment has been minimized.

focusaurus commented Aug 20, 2012

OK, so I ran the relevant snippet on Python2.5 and the basic try/import/except/return block works as aspected. Any insight into why this might be hanging and timing out on python 2.5? In any case, I don't think this specific test is that important, so if you can merge this pull request as is now (assuming it passes Travis), that would be great. Would like to understand the 2.5 failure, but willing to abandon it as low value if needed.

@travisbot

This comment has been minimized.

travisbot commented Aug 20, 2012

This pull request passes (merged 8dc2538 into 994a76e).

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 20, 2012

I'd guess that the hang might be related to something within the test framework (nose), or to some 2.5 specific bug fixed by 2.7. I don't think it's worth chasing down right now, given that your other tests prove most of the functionality.

Can you update the docs too, please? All of the below should have existing text you can use as an example.

  • put/get docstrings, including .. versionadded: blocks (for version 1.5)
  • Entry in docs/changelog.rst crediting yourself however you wish
@travisbot

This comment has been minimized.

travisbot commented Aug 21, 2012

This pull request passes (merged 5a73e91 into 994a76e).

@focusaurus

This comment has been minimized.

focusaurus commented Aug 27, 2012

Just a bump. I think this should be ready to be merged.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 27, 2012

I think this is the first 'bump' in a long time that was 100% legit, as in: yes this is ready for a merge and I somehow missed out on the notification from your latest update, my bad. Thanks!

bitprophet added a commit that referenced this pull request Aug 27, 2012

Merge pull request #699 from focusaurus/master
Allow get/put to use name attribute of StringIO/BytesIO objects in output

@bitprophet bitprophet merged commit e12300b into fabric:master Aug 27, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment