Greg Stein wrote on Wed, 07 Aug 2019 05:28 +00:00:
> On Tue, Aug 6, 2019 at 9:41 AM Daniel Shahaf <d...@daniel.shahaf.name> wrote:
> > Greg Stein wrote on Tue, 06 Aug 2019 07:58 +00:00:
> > > On Fri, Aug 2, 2019 at 12:53 PM <danie...@apache.org> wrote:
> > > >...
> > > > +++ subversion/site/tools/upcoming.py Fri Aug 2 17:53:38 2019
> > > >...
> > > > +def get_reference_version():
> > > > + "Return the version to use as the oldest end of the 'svn log' output
> > to generate."
> > > > + def _is_working_copy():
> > > > + return os.path.exists('subversion/include/svn_version.h')
> > > > + if _is_working_copy():
> > >
> > > Why a local func instead of just using os.path.exists() in the 'if'
> > > statement? If for doc purposes, then I think a comment would suffice.
> >
> > Yes, for doc purposes. How would a comment be better than a one-line
> > helper function?
>
> For me, it was the complexity. "Oh! A local function. What is this
> gonna be used for? ... oh." Local functions are a pretty high-level
> Python Fu. It kinda stood out to me.
>
> It is a rather complicated commentary. I'd think a simpler approach:
> # are we looking at a local working copy?
I understand what you're saying, but I don't see it this way. I find
the incumbent code clear, and I also find it clearer than it would be
with the function inlined and a comment added. I suppose it boils down
to two things: I find «if _is_working_copy():» more readable than the
alternative, and I don't see «def» at inner scopes as high-level fu.
Cheers,
Daniel
P.S. Have you seen the assignment to Version.__str__ on line 28?
