[Twisted-Python] Re: [Twisted-commits] r22428 - Deprecation API for functions and methods.
On Wed, 30 Jan 2008 18:51:09 -0700, Jonathan Lange <jml@wolfwood.twistedmatrix.com> wrote:
[snip]
Modified: trunk/twisted/python/versions.py ============================================================================== --- trunk/twisted/python/versions.py (original) +++ trunk/twisted/python/versions.py Wed Jan 30 18:51:07 2008 @@ -122,3 +122,14 @@ if ver is None: return '' return ' (SVN r%s)' % (ver,) + + + +def getVersionString(version): + """ + Get a friendly string for the given version object. + + @param version: A L{Version} object. + @return: A string containing the package and short version number. + """ + return '%s %s' % (version.package, version.short())
What's the right way to get a string describing a Version now? It's an even harder decision to make than it was before. Is it: * str(Version(...)) * repr(Version(...)) * Version(...).base() * Version(...).short() * getVersionString(Version(...)) Can we do something to make this better? Jean-Paul
On 02:45 pm, exarkun@divmod.com wrote:
What's the right way to get a string describing a Version now? It's an even harder decision to make than it was before. Is it:
* str(Version(...)) * repr(Version(...)) * Version(...).base() * Version(...).short() * getVersionString(Version(...))
Can we do something to make this better?
This collection is super ad-hoc. Let me try to express some of the requirements that lead to the current confusion: * Sometimes you want a string that is formatted for use at a REPL, which can be eval'd to give the user the idea that it is a structured object and how they can build something similar (repr()) * sometimes you want something that's human-readable and expresses all the information available about the version (str()). The square brackets probably come from the fact that I was playing WoW at the time, and square brackets meant "this is a thing, not just something a guy said". I was thinking you'd want to see like [Twisted Epic Spaulders of the Owl] for the version number. (Maybe a bad call.) * Sometimes you want just the version number in canonical format (x.y.z+dev) not the name of the package (.short()) * I hypothesize that sometimes you want to omit the +dev part because you're trying to fit into a stricter format, but I don't really remember why .base() exists. Was getVersionString added because the other string representations weren't "friendly" enough? I guess the 'rUnknown' makes them look a little gnarly, but I'd actually like to see the SVN revision in the cases where it's present; the right thing to do to make it friendly would be to fix the entries-file parser. Also, in any case, it should really be a method. There's no reason to have a free function defined right after a class whose only argument is a single instance of that class :).
On Thu, 31 Jan 2008 16:11:10 -0000, glyph@divmod.com wrote:
On 02:45 pm, exarkun@divmod.com wrote:
What's the right way to get a string describing a Version now? It's an even harder decision to make than it was before. Is it:
* str(Version(...)) * repr(Version(...)) * Version(...).base() * Version(...).short() * getVersionString(Version(...))
Can we do something to make this better?
This collection is super ad-hoc. Let me try to express some of the requirements that lead to the current confusion:
* [snip - special-case view requirement] * [snip - another special-case view requirement] * [snip - another special-case view requirement] * [snip - another special-case view requirement]
Was getVersionString added because the other string representations weren't "friendly" enough? I guess the 'rUnknown' makes them look a little gnarly, but I'd actually like to see the SVN revision in the cases where it's present; the right thing to do to make it friendly would be to fix the entries-file parser.
Also, in any case, it should really be a method. There's no reason to have a free function defined right after a class whose only argument is a single instance of that class :).
Maybe Version shouldn't be responsible for any view at all. Maybe Version is just a model class, and things that actually know what the view requirements are can implement the view? Jean-Paul
On 04:23 pm, exarkun@divmod.com wrote:
Maybe Version shouldn't be responsible for any view at all. Maybe Version is just a model class, and things that actually know what the view requirements are can implement the view?
I agree insofar as the model is poorly exposed and documented. The ivars have no documentation, and the only way to get the SVN revision is to call a private method or to mangle strings. However, some of these "views" should definitely be attached to the class, for convenience and simplicity. For example, a sensible __repr__ makes debugging and inspecting these things a lot easier. Adding a new module, function, or class which does the string formatting means that users of Version will need to look somewhere else to perform the function they want, and ultimately it doesn't eliminate the confusion - "do I want the 'short dotted' view or the 'includes project but without development version' view?" - unless we also reduce the number of options available.
On Thu, 31 Jan 2008 17:01:48 -0000, glyph@divmod.com wrote:
On 04:23 pm, exarkun@divmod.com wrote:
Maybe Version shouldn't be responsible for any view at all. Maybe Version is just a model class, and things that actually know what the view requirements are can implement the view?
I agree insofar as the model is poorly exposed and documented. The ivars have no documentation, and the only way to get the SVN revision is to call a private method or to mangle strings.
However, some of these "views" should definitely be attached to the class, for convenience and simplicity. For example, a sensible __repr__ makes debugging and inspecting these things a lot easier.
Having Version.__repr__ is fine. __repr__ is a thing that Python classes often have.
Adding a new module, function, or class which does the string formatting means that users of Version will need to look somewhere else to perform the function they want, and ultimately it doesn't eliminate the confusion - "do I want the 'short dotted' view or the 'includes project but without development version' view?" - unless we also reduce the number of options available.
What I had in mind was more that each application that wants to format Version in some particular way can do the formatting itself, not that it would pick an appropriate function from the version_view module and rely on it to do the formatting. The complexity of the code we're talking about foisting on an application is on the order of "%s.%s" % (version.major, version.minor). Not much of a burden. On the other hand, if we really want to make this easy, then maybe Version should have a `format´ method that just takes a format string and does the interpolation for you. Jean-Paul
Jean-Paul Calderone wrote:
On Thu, 31 Jan 2008 17:01:48 -0000, glyph@divmod.com wrote:
On 04:23 pm, exarkun@divmod.com wrote:
Maybe Version shouldn't be responsible for any view at all. Maybe Version is just a model class, and things that actually know what the view requirements are can implement the view?
I agree insofar as the model is poorly exposed and documented. The ivars have no documentation, and the only way to get the SVN revision is to call a private method or to mangle strings.
However, some of these "views" should definitely be attached to the class, for convenience and simplicity. For example, a sensible __repr__ makes debugging and inspecting these things a lot easier.
Having Version.__repr__ is fine. __repr__ is a thing that Python classes often have.
Adding a new module, function, or class which does the string formatting means that users of Version will need to look somewhere else to perform the function they want, and ultimately it doesn't eliminate the confusion - "do I want the 'short dotted' view or the 'includes project but without development version' view?" - unless we also reduce the number of options available.
What I had in mind was more that each application that wants to format Version in some particular way can do the formatting itself, not that it would pick an appropriate function from the version_view module and rely on it to do the formatting.
The complexity of the code we're talking about foisting on an application is on the order of "%s.%s" % (version.major, version.minor). Not much of a burden. On the other hand, if we really want to make this easy, then maybe Version should have a `format´ method that just takes a format string and does the interpolation for you.
And gold-plated toilet seats, they're good too ... regards Steve -- Steve Holden +1 571 484 6266 +1 800 494 3119 Holden Web LLC http://www.holdenweb.com/
On Feb 1, 2008 3:23 AM, Jean-Paul Calderone <exarkun@divmod.com> wrote:
On Thu, 31 Jan 2008 16:11:10 -0000, glyph@divmod.com wrote:
On 02:45 pm, exarkun@divmod.com wrote:
What's the right way to get a string describing a Version now? It's an even harder decision to make than it was before. Is it:
* str(Version(...)) * repr(Version(...)) * Version(...).base() * Version(...).short() * getVersionString(Version(...))
Can we do something to make this better?
This collection is super ad-hoc. Let me try to express some of the requirements that lead to the current confusion:
* [snip - special-case view requirement] * [snip - another special-case view requirement] * [snip - another special-case view requirement] * [snip - another special-case view requirement]
Was getVersionString added because the other string representations weren't "friendly" enough? I guess the 'rUnknown' makes them look a little gnarly, but I'd actually like to see the SVN revision in the cases where it's present; the right thing to do to make it friendly would be to fix the entries-file parser.
Also, in any case, it should really be a method. There's no reason to have a free function defined right after a class whose only argument is a single instance of that class :).
Maybe Version shouldn't be responsible for any view at all. Maybe Version is just a model class, and things that actually know what the view requirements are can implement the view?
That was my thinking. Originally, getVersionString was in t.p.deprecate, but I moved it on reviewer suggestion. jml
On 31 Jan, 11:44 pm, jml@mumak.net wrote:
Maybe Version shouldn't be responsible for any view at all. Maybe Version is just a model class, and things that actually know what the view requirements are can implement the view?
That was my thinking. Originally, getVersionString was in t.p.deprecate, but I moved it on reviewer suggestion.
I'd really prefer it if we had a canonical string representation for versions which t.p.deprecate would use as well as anything else which needs to report information about a version to developers. The (existing?) str() ought to be acceptable for that. That way when you saw a string in a certain format you could immediately identify it as a precise statement about a current or future Twisted package version. I was sort of hoping that we might have Version-alike objects at some point which could know about different versioning schemes, and ways of comparing different revision control systems (although I guess dealing with comparing arbitrary bzr revisions in one tiny little object might be too much to ask...). This is one reason I don't want to separate the view out too much; inspecting the model directly binds any potential view very tightly to the x.y.z versioning scheme. But, this is somewhat obviously a bike shed, so I am not going to agitate terribly hard for this. It's not like I use Twisted primarily for dealing with my obscurely formatted package version strings ;-).
On Feb 1, 2008 1:19 PM, <glyph@divmod.com> wrote:
On 31 Jan, 11:44 pm, jml@mumak.net wrote:
Maybe Version shouldn't be responsible for any view at all. Maybe Version is just a model class, and things that actually know what the view requirements are can implement the view?
That was my thinking. Originally, getVersionString was in t.p.deprecate, but I moved it on reviewer suggestion.
I'd really prefer it if we had a canonical string representation for versions which t.p.deprecate would use as well as anything else which needs to report information about a version to developers. The (existing?) str() ought to be acceptable for that. That way when you saw a string in a certain format you could immediately identify it as a precise statement about a current or future Twisted package version.
Having a canonical representation doesn't exclude having multiple representations. Looking at how these are actually used: - str() is used only for copyright (and tests) - short is used for __init__.py - base is used for short, in _release._changeVersionInFile (don't know why base rather than everyone else, actually) and dist.py - repr is only used generically. - getVersionString is used only by deprecate. In summary, sometimes we want the project name, sometimes we want the svn revno. This is perhaps better achieved by having a single function / method that takes a couple of params. Personally, I think that having a project name in Version is silly.
I was sort of hoping that we might have Version-alike objects at some point which could know about different versioning schemes, and ways of comparing different revision control systems (although I guess dealing with comparing arbitrary bzr revisions in one tiny little object might be too much to ask...). This is one reason I don't want to separate the view out too much; inspecting the model directly binds any potential view very tightly to the x.y.z versioning scheme.
For the purposes of this discussion, that is a massive, massive YAGNI.
But, this is somewhat obviously a bike shed, so I am not going to agitate terribly hard for this. It's not like I use Twisted primarily for dealing with my obscurely formatted package version strings ;-).
Right. I propose we do nothing. jml
On 04:34 am, jml@mumak.net wrote:
On Feb 1, 2008 1:19 PM, <glyph@divmod.com> wrote:
Having a canonical representation doesn't exclude having multiple representations.
True enough.
Looking at how these are actually used: - str() is used only for copyright (and tests)
This is probably the most poorly-thought-out method of the bunch anyway, so I'm glad to see it's used less.
- short is used for __init__.py
Makes sense, this is where you'd go looking.
- base is used for short, in _release._changeVersionInFile (don't know why base rather than everyone else, actually) and dist.py
base is used here because we will explicitly never put an SVN rev into a README file.
- getVersionString is used only by deprecate.
So, perhaps this and str() should be the same thing. Is there really a reason for omitting the svn revno here?
In summary, sometimes we want the project name, sometimes we want the svn revno. This is perhaps better achieved by having a single function / method that takes a couple of params.
Personally, I think that having a project name in Version is silly.
... and yet, getVersionString uses that attribute? The point of having a project name in a version is to specifically qualify the version to alert about meaningless comparisons. "Twisted version 2.5.0" does not meaningfully compare to "Python version 2.5.0", despite being apparently equal if you elide the project name.
For the purposes of this discussion, (my insane idea) is a massive, massive YAGNI.
Definitely.
But, this is somewhat obviously a bike shed, so I am not going to agitate terribly hard for this. It's not like I use Twisted primarily for dealing with my obscurely formatted package version strings ;-).
Right. I propose we do nothing.
An easy position to defend.
On Feb 1, 2008 4:11 PM, <glyph@divmod.com> wrote:
On 04:34 am, jml@mumak.net wrote:
On Feb 1, 2008 1:19 PM, <glyph@divmod.com> wrote:
Having a canonical representation doesn't exclude having multiple representations.
True enough.
Looking at how these are actually used: - str() is used only for copyright (and tests)
This is probably the most poorly-thought-out method of the bunch anyway, so I'm glad to see it's used less.
- short is used for __init__.py
Makes sense, this is where you'd go looking.
- base is used for short, in _release._changeVersionInFile (don't know why base rather than everyone else, actually) and dist.py
base is used here because we will explicitly never put an SVN rev into a README file.
- getVersionString is used only by deprecate.
So, perhaps this and str() should be the same thing. Is there really a reason for omitting the svn revno here?
Is there a reason for including it? getVersionString should probably be str()
In summary, sometimes we want the project name, sometimes we want the svn revno. This is perhaps better achieved by having a single function / method that takes a couple of params.
Personally, I think that having a project name in Version is silly.
... and yet, getVersionString uses that attribute?
Well, it seemed like going with the flow. The alternative was to hard-code the word 'Twisted' into the deprecation code -- something that I wouldn't mind at all.
The point of having a project name in a version is to specifically qualify the version to alert about meaningless comparisons. "Twisted version 2.5.0" does not meaningfully compare to "Python version 2.5.0", despite being apparently equal if you elide the project name.
And the number of times we've constructed a twisted Version object for Python is how many?
But, this is somewhat obviously a bike shed, so I am not going to agitate terribly hard for this. It's not like I use Twisted primarily for dealing with my obscurely formatted package version strings ;-).
Right. I propose we do nothing.
An easy position to defend.
Its virtue is its clarity. I fall from my earlier motion and move that we move make Version.__str__ behave like getVersionString and delete getVersionString. Or, see the attached patch. I'm off to a party where the drinks are on Google. jml
On 07:46 am, jml@mumak.net wrote:
On Feb 1, 2008 4:11 PM, <glyph@divmod.com> wrote:
On 04:34 am, jml@mumak.net wrote:
- getVersionString is used only by deprecate.
So, perhaps this and str() should be the same thing. Is there really a reason for omitting the svn revno here?
Is there a reason for including it?
Diagnosing warnings and errors from development versions is easier if they properly report that they're development versions. The SVN version will be omitted if it's not discovered, anyway. (Though the SVN version discovery code needs to be fixed up, a separate issue.)
getVersionString should probably be str()
Cool.
The point of having a project name in a version is to specifically qualify the version to alert about meaningless comparisons. "Twisted version 2.5.0" does not meaningfully compare to "Python version 2.5.0", despite being apparently equal if you elide the project name.
And the number of times we've constructed a twisted Version object for Python is how many?
Sorry, a better example would be "'Twisted version 2.5.0' does not meaningfully compare to 'Axiom version 2.5.0'". And that's where this versioning code came from in the first place, so yes, we've built one or two of those objects :-).
But, this is somewhat obviously a bike shed, so I am not going to agitate terribly hard for this. It's not like I use Twisted primarily for dealing with my obscurely formatted package version strings ;-).
Right. I propose we do nothing.
An easy position to defend.
Its virtue is its clarity.
I fall from my earlier motion and move that we move make Version.__str__ behave like getVersionString and delete getVersionString. Or, see the attached patch.
That also works for me.
I'm off to a party where the drinks are on Google.
Cool, have a good time. But next time, try to score a party where the *cars* are on google. With their accounting they might not be able to tell the difference!
On Thu, 2008-01-31 at 16:11 +0000, glyph@divmod.com wrote:
On 02:45 pm, exarkun@divmod.com wrote:
What's the right way to get a string describing a Version now? It's an even harder decision to make than it was before. Is it:
* str(Version(...)) * repr(Version(...)) * Version(...).base() * Version(...).short() * getVersionString(Version(...))
Can we do something to make this better?
This collection is super ad-hoc. Let me try to express some of the requirements that lead to the current confusion:
* Sometimes you want a string that is formatted for use at a REPL, which can be eval'd to give the user the idea that it is a structured object and how they can build something similar (repr()) * sometimes you want something that's human-readable and expresses all the information available about the version (str()). The square brackets probably come from the fact that I was playing WoW at the time, and square brackets meant "this is a thing, not just something a guy said". I was thinking you'd want to see like [Twisted Epic Spaulders of the Owl] for the version number. (Maybe a bad call.) * Sometimes you want just the version number in canonical format (x.y.z+dev) not the name of the package (.short()) * I hypothesize that sometimes you want to omit the +dev part because you're trying to fit into a stricter format, but I don't really remember why .base() exists.
Was getVersionString added because the other string representations weren't "friendly" enough? I guess the 'rUnknown' makes them look a little gnarly, but I'd actually like to see the SVN revision in the cases where it's present; the right thing to do to make it friendly would be to fix the entries-file parser.
Also, in any case, it should really be a method. There's no reason to have a free function defined right after a class whose only argument is a single instance of that class :).
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python -- George Pauly Ring Development www.ringdevelopment.com
participants (5)
-
George Pauly -
glyph＠divmod.com -
Jean-Paul Calderone -
Jonathan Lange -
Steve Holden