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

NamedStream cleanup #657

Merged
merged 5 commits into from
Jan 22, 2016
Merged

NamedStream cleanup #657

merged 5 commits into from
Jan 22, 2016

Conversation

orbeckst
Copy link
Member

As discussed in #649 and PR #652, NamedStream is a bit of a hack... not necessarily in the best sense of the word. This PR builds on #652 (towards Py3 compatibility) and tries to make NamedStream work as seamlessly as advertised or at least document its failures.

I added tests that explore the limitations of using a NamedStream instance as a filename under the new scheme where the class is not derived from either basestring or str. I apply most of the filename-related tests in os.path.

The tests partially fail on those that have to do a string concatenation with NamedString as the argument, i.e.

  str + NamedStream

which is

 str.__add__(NamedStream)

That seems to be a problem that can only be resolved by properly inheriting from something that str.__add__ is willing to consume.

UPDATE: Below @richardjgowers rightly points out that NamedStream.__radd__ will solve the problem.

@richardjgowers
Copy link
Member

This should fix the last problem

    def __radd__(self, other):
        return other + self.name

It overrides the radd of NamedStream, so when str.__add__ fails it looks for NamedStream.__radd__

@richardjgowers
Copy link
Member

Oh wait, I've just seen you've added radd, which is weird because I got:

In [2]: s = mda.lib.util.NamedStream(open('./two_water_gro.gro'), 'magicgro')

In [3]: 'this' + s
Out[3]: 'thismagicgro'

@orbeckst
Copy link
Member Author

__radd__ works! I had not included it because I did not see it in a native string via introspection.

I am just doing an amend... incoming.

…and radd

This enables full string addition, both string + NamedStream and NamedStream + string.
Many of the os.path functions will now work with NamedStream.

(Thanks to @richardjgowers for __radd__!)
- test expanduser: works but behaves unexpectedly (but consistently) in that it
  returns the expanded string if a tilde was in the name but does nothing (as
  described in the docs) when it does not do a substitution, which means that
  it now returns the original NamedString. Kind of messy but probably tolerable.
- test expandvars: does not work
- note: some of the tests related to expandvars and expandusers generated
        Segmentation fault on my Mac OS X 10.6 with Python 2.7.11 and I disabled
        the tests.
@orbeckst orbeckst force-pushed the feature-NamedStream-hacks branch from ec2ceac to 92464cd Compare January 22, 2016 20:11
@orbeckst
Copy link
Member Author

Most of the os.path functions work except

  • expandvars just returns NamedStream without doing anything
  • expanduser works in that it does return a string with ~ expanded but if it does no expansions it returns the NamedStream. In principle this is acceptable behavior but unexpected; it should work (tm) because downstream string processing should be dealt with correctly with the NamedStream.

Unfortunately, my tests segmentation faulted for expandvars and non-expanding expanduser so I skip them at the moment...

@richardjgowers richardjgowers self-assigned this Jan 22, 2016
@richardjgowers richardjgowers added this to the 0.14 milestone Jan 22, 2016
richardjgowers added a commit that referenced this pull request Jan 22, 2016
@richardjgowers richardjgowers merged commit f13d8c8 into develop Jan 22, 2016
@richardjgowers richardjgowers deleted the feature-NamedStream-hacks branch January 22, 2016 22:35
@richardjgowers
Copy link
Member

Another py3 enemy bites the dust, awesome

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