You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by hw...@apache.org on 2011/08/10 23:26:36 UTC
svn commit: r1156375 - /subversion/branches/fs-py/subversion/python/svn/fs.py
Author: hwright
Date: Wed Aug 10 21:26:36 2011
New Revision: 1156375
URL: http://svn.apache.org/viewvc?rev=1156375&view=rev
Log:
On the fs-py branch:
Followup to r1156347: use a bit cleaner syntax to automagically close files.
* subversion/python/svn/fs.py
(FS.set_uuid, FS._open_fs): Use the with statement.
Modified:
subversion/branches/fs-py/subversion/python/svn/fs.py
Modified: subversion/branches/fs-py/subversion/python/svn/fs.py
URL: http://svn.apache.org/viewvc/subversion/branches/fs-py/subversion/python/svn/fs.py?rev=1156375&r1=1156374&r2=1156375&view=diff
==============================================================================
--- subversion/branches/fs-py/subversion/python/svn/fs.py (original)
+++ subversion/branches/fs-py/subversion/python/svn/fs.py Wed Aug 10 21:26:36 2011
@@ -35,9 +35,8 @@ class FS(object):
uuid_in = uuid.uuid1()
self.uuid = uuid_in
- f = open(self.__path_uuid, 'wb')
- f.write(str(self.uuid) + '\n')
- f.close()
+ with open(self.__path_uuid, 'wb') as f:
+ f.write(str(self.uuid) + '\n')
# We use the permissions of the 'current' file, because the 'uuid'
# file does not exist during repository creation.
@@ -50,9 +49,8 @@ class FS(object):
def _open_fs(self):
'Open an existing Subvesion filesystem'
- f = open(self.__path_uuid, 'rb')
- self.uuid = uuid.UUID(f.readline().rstrip())
- f.close()
+ with open(self.__path_uuid, 'rb') as f:
+ self.uuid = uuid.UUID(f.readline().rstrip())
def __setup_paths(self):
Re: svn commit: r1156375 - /subversion/branches/fs-py/subversion/python/svn/fs.py
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Wed, Aug 10, 2011 at 6:42 PM, Greg Stein <gs...@gmail.com> wrote:
> On Wed, Aug 10, 2011 at 17:44, Hyrum K Wright <hy...@wandisco.com> wrote:
>>...
>>> And now where did your .close() call go? You're still relying on 'f'
>>> going out of scope to close. IOW, right back to the zero refcount
>>> algorithm. And in which case, the original construction is much
>>> cleaner than all this "with" gunk.
>>
>> In the case of an open()'d file, the file is closed when the object is
>> eventually garbage collected, which, by documentation[1] isn't
>> guaranteed to happen when the object is unreachable: "Do not depend on
>> immediate finalization of objects when they become unreachable (ex:
>> always close files)." Hence the need for an explicit close.
>
> That is referring to circular references, where all objects in the
> circle are (no longer) referenced from outside the circle.
>
> Given:
>
> open(file).read()
>
> You get a reference to the file, it is stashed into a bound method
> object, the method is invoked with another reference increasing as the
> file is bound to 'self'. When the method returns, the bound method is
> decref'd which decref's the file, which closes it.
Correct. This is not a circular reference, but rather uses an
anonymous object or two.
The general point is that relying upon an implementation detail as to
the timeliness of the GC is probably not a good idea. Although the
docs I cited did refer to circular references, I think the general
point about not relying upon implementation details is a valid one.
(One of the benefits of a Python implementation is that somebody could
run it in CPython or PyPy or IronPython or Jython or Cython or $FOO.
I'd rather not make assumptions which limit those options when we
don't have to.)
>>...
>> As for "gunk," the with construction was introduced in Python 2.5,
>> almost 5 years ago and I find it cleaner.
>
> I find the use of control flow construct to replace a simple
> expression... to be more complicated. Has no bearing on how long
> 'with' has been around.
Fair enough. I feel the nature of the with syntax lends itself to
better comprehension of what's going on. We can agree to disagree. :)
> And now that you mention it... we have always said that our delivered
> Python code (as distinct from our build/test environment) should be
> compatible with Python 2.4. I don't recall that we've changed that
> position.
Actually, we don't require users to have *any* version of Python
installed to run Subversion. If you install a vanilla Subversion
client from your local package manager, it's not going to pull in
Python as a run-time dependency.
As this (still highly experimental*) work would obviously introduce
that dependency, and requiring something as ancient as Python 2.5
isn't further adding to that burden.
Oh, and for the record, I didn't interpret Greg's comments as jumping
on me, though I see how others could. I've learned to appreciate (and
deal with) his rather direct style in reviewing code.
-Hyrum
* Let's not forget that this is an experimental branch with (in my
opinion) a low probability of ever being release as part of the core
product.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: svn commit: r1156375 - /subversion/branches/fs-py/subversion/python/svn/fs.py
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Aug 10, 2011 at 17:44, Hyrum K Wright <hy...@wandisco.com> wrote:
>...
>> And now where did your .close() call go? You're still relying on 'f'
>> going out of scope to close. IOW, right back to the zero refcount
>> algorithm. And in which case, the original construction is much
>> cleaner than all this "with" gunk.
>
> In the case of an open()'d file, the file is closed when the object is
> eventually garbage collected, which, by documentation[1] isn't
> guaranteed to happen when the object is unreachable: "Do not depend on
> immediate finalization of objects when they become unreachable (ex:
> always close files)." Hence the need for an explicit close.
That is referring to circular references, where all objects in the
circle are (no longer) referenced from outside the circle.
Given:
open(file).read()
You get a reference to the file, it is stashed into a bound method
object, the method is invoked with another reference increasing as the
file is bound to 'self'. When the method returns, the bound method is
decref'd which decref's the file, which closes it.
>...
> As for "gunk," the with construction was introduced in Python 2.5,
> almost 5 years ago and I find it cleaner.
I find the use of control flow construct to replace a simple
expression... to be more complicated. Has no bearing on how long
'with' has been around.
And now that you mention it... we have always said that our delivered
Python code (as distinct from our build/test environment) should be
compatible with Python 2.4. I don't recall that we've changed that
position.
Cheers,
-g
Re: svn commit: r1156375 - /subversion/branches/fs-py/subversion/python/svn/fs.py
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Wed, Aug 10, 2011 at 4:35 PM, Greg Stein <gs...@gmail.com> wrote:
> On Wed, Aug 10, 2011 at 17:26, <hw...@apache.org> wrote:
>> Author: hwright
>> Date: Wed Aug 10 21:26:36 2011
>> New Revision: 1156375
>>
>> URL: http://svn.apache.org/viewvc?rev=1156375&view=rev
>> Log:
>> On the fs-py branch:
>> Followup to r1156347: use a bit cleaner syntax to automagically close files.
>>
>> * subversion/python/svn/fs.py
>> (FS.set_uuid, FS._open_fs): Use the with statement.
>
> And now where did your .close() call go? You're still relying on 'f'
> going out of scope to close. IOW, right back to the zero refcount
> algorithm. And in which case, the original construction is much
> cleaner than all this "with" gunk.
In the case of an open()'d file, the file is closed when the object is
eventually garbage collected, which, by documentation[1] isn't
guaranteed to happen when the object is unreachable: "Do not depend on
immediate finalization of objects when they become unreachable (ex:
always close files)." Hence the need for an explicit close.
However, the "with" statement[2] used in this revision *does*
guarantee that the file will be closed immediately upon completion of
the block, even in the case that an exception is raised in the block.
As for "gunk," the with construction was introduced in Python 2.5,
almost 5 years ago and I find it cleaner.
-Hyrum
[1] http://docs.python.org/reference/datamodel.html#objects-values-and-types
[2] http://docs.python.org/reference/datamodel.html#context-managers
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: AW: svn commit: r1156375 - /subversion/branches/fs-py/subversion/python/svn/fs.py
Posted by Greg Stein <gs...@gmail.com>.
On Aug 11, 2011 2:37 AM, "Markus Schaber" <m....@3s-software.com> wrote:
>
> Hi,
>
> Von: Greg Stein [mailto:gstein@gmail.com]
> ...
> > With the prior constructions:
> >
> > open(foo).read()
> > open(foo, 'w').write(bar)
>
> Constructs like this never worked reliable in alternative python
implementations.
Yes. I do understand this thought, but the bindings simply are not intended
for anything but CPython. If they somehow work for the other
implementations, then I'd be completely surprised.
> And now, as IronPython has a working cTypes implementation, maybe that is
the way to provide subversion support for IronPython.
There is some experimental work using ctypesgen-based bindings. I did a
small amount of work on them a couple years ago, but it could use more work.
That would definitely be interesting work if it could make svn available to
IronPython.
Cheers,
-g
AW: svn commit: r1156375 - /subversion/branches/fs-py/subversion/python/svn/fs.py
Posted by Markus Schaber <m....@3s-software.com>.
Hi,
Von: Greg Stein [mailto:gstein@gmail.com]
> On Wed, Aug 10, 2011 at 19:15, Branko Čibej <br...@xbc.nu> wrote:
> >...
> > Explicit management of rare resources is no bad thing. The "with"
> > statement was introduced specifically to do away with the flaky, three
> >times longer try-except-else that would otherwise be necessary in order
> >to guarantee that file objects are closed.
>
> With the prior constructions:
>
> open(foo).read()
> open(foo, 'w').write(bar)
Constructs like this never worked reliable in alternative python implementations.
And now, as IronPython has a working cTypes implementation, maybe that is the way to provide subversion support for IronPython.
Best regards
Markus Schaber
___________________________
We software Automation.
3S-Smart Software Solutions GmbH
Markus Schaber | Developer
Memminger Str. 151 | 87439 Kempten | Germany | Tel. +49-831-54031-0 | Fax +49-831-54031-50
Email: m.schaber@3s-software.com | Web: http://www.3s-software.com
CoDeSys internet forum: http://forum.3s-software.com
Download CoDeSys sample projects: http://www.3s-software.com/index.shtml?sample_projects
Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner | Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
Re: svn commit: r1156375 - /subversion/branches/fs-py/subversion/python/svn/fs.py
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Aug 10, 2011 at 19:15, Branko Čibej <br...@xbc.nu> wrote:
>...
> Explicit management of rare resources is no bad thing. The "with"
> statement was introduced specifically to do away with the flaky, three
> times longer try-except-else that would otherwise be necessary in order
> to guarantee that file objects are closed.
With the prior constructions:
open(foo).read()
open(foo, 'w').write(bar)
If one of those .read() or .write() calls fails, *and* somebody does
not clear the traceback... then yeah. A file remains open, referenced
from the traceback. But somebody has to be explicitly *keeping* those
tracebacks. I've never seen code where people keep lists of
tracebacks. I cannot even speculate on why somebody might use svn.fs,
capture tracebacks, hold onto them, and then *continue* trying to use
svn.fs.
Switching to:
with open(foo) as f:
f.read()
You're going from a single expression to a control flow construct,
simply to try and avoid a situation that I doubt anybody could even
begin to argue would come up.
> And yes, the CPython docs do say that file objects are closed when their
> refcount reaches zero, but also state that one shouldn't rely on that.
> Also see the second stanza in PEP 20. :)
Ha! And the third stanza says "Simple is better than complex."
> There's no call to jump all over people because you don't agree with
> their coding style.
If you call that "jump[ing] all over people", then I think you're
taking it wrong. I'm providing review feedback where I see a simple
construction being turned into something more complicated.
Cheers,
-g
Re: svn commit: r1156375 - /subversion/branches/fs-py/subversion/python/svn/fs.py
Posted by Branko Čibej <br...@xbc.nu>.
On 10.08.2011 23:35, Greg Stein wrote:
> On Wed, Aug 10, 2011 at 17:26, <hw...@apache.org> wrote:
>> Author: hwright
>> Date: Wed Aug 10 21:26:36 2011
>> New Revision: 1156375
>>
>> URL: http://svn.apache.org/viewvc?rev=1156375&view=rev
>> Log:
>> On the fs-py branch:
>> Followup to r1156347: use a bit cleaner syntax to automagically close files.
>>
>> * subversion/python/svn/fs.py
>> (FS.set_uuid, FS._open_fs): Use the with statement.
> And now where did your .close() call go? You're still relying on 'f'
> going out of scope to close. IOW, right back to the zero refcount
> algorithm. And in which case, the original construction is much
> cleaner than all this "with" gunk.
>
> ???
Greg, you should keep up with the times. :)
Explicit management of rare resources is no bad thing. The "with"
statement was introduced specifically to do away with the flaky, three
times longer try-except-else that would otherwise be necessary in order
to guarantee that file objects are closed.
And yes, the CPython docs do say that file objects are closed when their
refcount reaches zero, but also state that one shouldn't rely on that.
Also see the second stanza in PEP 20. :)
There's no call to jump all over people because you don't agree with
their coding style.
-- Brane
Re: svn commit: r1156375 - /subversion/branches/fs-py/subversion/python/svn/fs.py
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Aug 10, 2011 at 17:26, <hw...@apache.org> wrote:
> Author: hwright
> Date: Wed Aug 10 21:26:36 2011
> New Revision: 1156375
>
> URL: http://svn.apache.org/viewvc?rev=1156375&view=rev
> Log:
> On the fs-py branch:
> Followup to r1156347: use a bit cleaner syntax to automagically close files.
>
> * subversion/python/svn/fs.py
> (FS.set_uuid, FS._open_fs): Use the with statement.
And now where did your .close() call go? You're still relying on 'f'
going out of scope to close. IOW, right back to the zero refcount
algorithm. And in which case, the original construction is much
cleaner than all this "with" gunk.
???