Skip to content

Improve __repr__#76

Merged
mgeier merged 3 commits intomasterfrom
repr
Jan 9, 2015
Merged

Improve __repr__#76
mgeier merged 3 commits intomasterfrom
repr

Conversation

@bastibe
Copy link
Owner

@bastibe bastibe commented Dec 10, 2014

__repr__ could include the file name, samplerate and number of channels, and possibly the open mode.

@mgeier
Copy link
Contributor

mgeier commented Oct 14, 2014

That's a great idea!
Not very high priority though ...

@mgeier mgeier modified the milestone: 0.7 Oct 26, 2014
@bastibe bastibe modified the milestones: 0.6.0, 0.6.x Nov 24, 2014
@bastibe bastibe self-assigned this Nov 24, 2014
@bastibe bastibe closed this in 98f2131 Nov 28, 2014
@mgeier
Copy link
Contributor

mgeier commented Dec 2, 2014

Currently, this doesn't work on Python 2.6, see my comments on 98f2131.

@mgeier mgeier reopened this Dec 2, 2014
@bastibe
Copy link
Owner Author

bastibe commented Dec 2, 2014

Sorry for the close. I confused my branches with this one.

@bastibe
Copy link
Owner Author

bastibe commented Dec 10, 2014

2.6 compatibility should be fixed now. I think that endian and subtype are not important for day-to-day work. Similarly, I think that the current solution with self.name works fine, since the user is probably very aware of how he opened that file anyway. I see the value of __repr__ more in getting a quick glance at the file, and less in actually providing constructor arguments.

pysoundfile.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to replace "%s" with %r.

This would use the typical quotes, in this case single quotes instead of double quotes.

@mgeier
Copy link
Contributor

mgeier commented Dec 10, 2014

I think that endian and subtype are not important for day-to-day work.

I think we should try, wherever possible, to raise awareness about the subtype.

When people save float arrays to WAV files, they will loose precision by default.
I don't think this is really a problem, but people should be aware of it.
And __repr__() is one of the places where we could tell them ...

I think that the current solution with self.name works fine, since the user is probably very aware of how he opened that file anyway.

Sure, it works, but it may not be the most appropriate piece of information to return.

I think it would be best if we change self.name to hold the repr() of the file object, then it will automatically be displayed in SoundFile.__repr__().
We'll have to modify the code for detecting the file extension, because it relies on self.name holding the file name and not the repr() of the file object.

We can also do this in a separate PR.

I see the value of __repr__ more in getting a quick glance at the file, and less in actually providing constructor arguments.

Me, too.

I think it's just more accurate and helpful to show the repr() of the file object.

And it's actually not providing a valid constructor argument, because it will be something like SoundFile(<_io.BufferedReader name='myfile.wav'>, ...).

@bastibe
Copy link
Owner Author

bastibe commented Dec 17, 2014

The file repr includes quotes, which messes up too many things. For the time being at least, name will not be changed.

Can we merge this?

@mgeier
Copy link
Contributor

mgeier commented Dec 17, 2014

The file repr includes quotes, which messes up too many things.

What exactly does that mess up?
I can't think of a single thing.

The file repr only contains single quotes, as should all our string fields (https://github.com/bastibe/PySoundFile/pull/76#discussion_r21603138).

Even if the repr would contain both kinds of quotes (which it doesn't, except the file name itself contains them) or our other strings would use double quotes (which they shouldn't), the string formatting mechanism would be intelligent enough to escape the quotes where necessary.

Can we merge this?

What about showing endian only when it's not equal to 'FILE' (bastibe/PySoundFile@98f2131#commitcomment-8769930)?

bastibe added a commit that referenced this pull request Dec 17, 2014
__repr__ now returns a nice description of the SoundFile
closes #76
@bastibe
Copy link
Owner Author

bastibe commented Dec 17, 2014

Re: repr: Apparently I do not know how you would like to incorporate repr into __repr__. Would you mind elaborating or implementing?

What about showing endian only when it's not equal to 'FILE'?

I am not in favor of this. As I said before, I would prefer not to show subtype or endian at all, since they will confuse more than they teach. endian in particular is probably not important to anyone, ever. I definitely don't want to conditionally show or hide information. Consistency is very important to me.

... instead of storing only the "name" attribute of the file-like object.

__repr__() shows the repr() of the file name/file descriptor/file-like
object and the repr() of the mode and format strings.
@mgeier
Copy link
Contributor

mgeier commented Dec 23, 2014

Would you mind elaborating or implementing?

Sure. I pushed a commit (5e9a075) which implements this.
Feel free to edit/remove it as you see fit.

Now, instead of holding the file name of the original file, SoundFile.name holds a reference to the actual file object. It doesn't hold its repr(), though, contrary to what I said in https://github.com/bastibe/PySoundFile/pull/76#issuecomment-66452863. This wouldn't make sense at all.

This is IMHO a consistent extension of the behavior of Python's file objects, where the name attribute holds an actual integer when using a file descriptor.
The __repr__() now shows the proper thing in each of the 3 cases: file name, file descriptor or file object.

As I said before, I would prefer not to show subtype or endian at all, since they will confuse more than they teach.

We have to draw the line somewhere ... but I think we shouldn't separate format from subtype.
I think they belong together. endian also belongs to the two, but I don't think people have to be bothered with it, except if it's different than the default.

What I want to say with that is, that if you want to drop subtype and endian (which would be OK), you should also drop format.

endian in particular is probably not important to anyone, ever.

True, except in the very unlikely case it's different from 'FILE', then it's quite important.

I definitely don't want to conditionally show or hide information. Consistency is very important to me.

That sounds like a good guideline.
But this is just one aspect of a more complex situation. As so often, we'll have to find a balance between different, often competing guidelines/rules/tastes/...

I think currently the two most promising solutions would be to show all three of format/subtype/endian or none of them.

I'd prefer former, which also happens to be the current state of this PR.

@mgeier
Copy link
Contributor

mgeier commented Dec 26, 2014

Just for reference: There might be some overlap with #58.

@bastibe
Copy link
Owner Author

bastibe commented Jan 8, 2015

OK, looks good to me. Merge if you agree.

I don't see why we need format should require subtype and endian, but if you insist on it, we'll do it that way. I'd rather have always too much information than a different amount of information for different files.

@bastibe bastibe assigned mgeier and unassigned bastibe Jan 8, 2015
mgeier added a commit that referenced this pull request Jan 9, 2015
OK, I'm merging this.

> I don't see why we need `format` should require `subtype` and `endian`, but if you insist on it, we'll do it that way. I'd rather have always too much information than a different amount of information for different files.

If we get annoyed by the large amount of information, we can still remove the `format`/`subtype`/`endian` triple at a later point.

Also, let's see if the implementation of #58 sheds a different light on this.
@mgeier mgeier merged commit 0b4b7c3 into master Jan 9, 2015
@mgeier
Copy link
Contributor

mgeier commented Jan 9, 2015

I inadvertently added this to the merge commit message, I actually wanted to post this comment here:

OK, I'm merging this.

I don't see why we need format should require subtype and endian, but if you insist on it, we'll do it that way. I'd rather have always too much information than a different amount of information for different files.

If we get annoyed by the large amount of information, we can still remove the format/subtype/endian triple at a later point.

Also, let's see if the implementation of #58 sheds a different light on this.

@bastibe bastibe deleted the repr branch April 25, 2016 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants