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 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

On Feb 1, 2008 3:23 AM, Jean-Paul Calderone 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 Feb 1, 2008 1:19 PM, 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 Feb 1, 2008 4:11 PM, wrote:

On 04:34 am, jml@mumak.net wrote:

...

On Feb 1, 2008 1:19 PM, 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 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