Merge lp:~vila/bzr/imports into lp:bzr

I'm not going to vote on this, but I would like to say that I'm still
totally unconvinced. I think that this is a flawed argument and an
accomodation to other design issues.

I've got a few point by point notes below.

+ The bad points of the ``from <module> import symbol`` style includes:
+
+ * The symbol list for each import is harder to keep up to date.

This isn't a compelling point: if optimising for that is a concern,
best to do 'from sys import modules' and access everything via
modules['bzrlib.foo.bar.baz'] - the ultimate in maintenance.

Really this is a tradeoff - its more repetition, more typing, and code
gets more verbose. Codifying one particular value on the tradeoff as
'right' is IMO a mistake.

+ * The imported symbols can be imported by other modules without realizing
+ the aliasing that is occurring.

This can happen if you use imports at all. 'from bzrlib.tests import
bzrlib as foo'.

The standard convention in the python world is to set __all__ to
declare what should be exported, and you can lint to warn. If such
aliasing is a concern. (I don't see why it matters - whats the
functional harm?).

+ * It forbids overriding the imported symbol for test purposes.

This is a dangerous testing approach anyway: changing the global
symbol table is a Big Hammer and should be avoided as much as
possible.

+ Or it
+ makes it harder and can lead to very obscure failures.

+ * Our `Lazy Imports`_ implementation can fail.

We should fix it then. It can still fail when importing modules,
though of course differently.

+ The good points of the ``<module>.symbol`` style includes:
+
+ * The import list at the top-level of the file is shorter hence easier to
+ read and provides a better overview of the relationship of the module
+ with the imported modules.

I don't agree that the overview is better: All the *important* detail
is now missing, and you have to do a line by line review of the rest
of the file to see whats up. Thats hardly an improvement.

+ * The code refers directly to the symbol used, avoiding a trip to the
+ top-level to know where the symbol is coming from. This applies both to
+ the casual reader and to the reviewers when a patch doesn't modify the
+ imports (which it the vast majority). In essence it makes the code
+ easier to understand while requiring typing more (the module prefix),
+ which is a Good Thing because the code is written once but read many
+ times.

This also makes code less flexible, you can't inject a new
implementation into that one module as easily. It also makes the code
less readable because its easily two or even three times as long to
express the same concept.

+ * It makes it easier to move code from one module to another. You don't
+ need to sort out which symbols need to be copied across the new module
+ or deleted in the old, you only have to care about modules (once).

If thats an issue, pyflakes or other things can sort that out pretty
darn quickly.

Frankly, if I was still hacking on bzr with any frequency I'd vote -1
on this : this should not be codified because making different
tradeoffs depending on what someone is doing is appropriate and
sen...

I'm glad you put this up, because I'd rather have the conversation
once and decide what we agree upon, than to have it as a side track
from some other change or even worse to have them flip too and fro.

typo 'drawbaks'.

I agree with Robert that this tends to make the code longer and all
else being equal we should resist that.

If this breaks lazy imports then ideally we would be fixing lazy
imports, or at worst documenting that things must be given in this
style specifically when they are lazily imported. I think lazy
imports are kind of a useful kludge anyhow, and not something that
should shift our idea of what is clearest.

One interesting case is 'ui.ui_factory' where we really do want to be
able to change the variable later. Perhaps this indicates that
pointing to it through a global variable is not ideal.

I think for things like 'errors.' where you do want to pull in
multiple classes it can be annoying to manually update the symbol
list.

It seems like updating or checking the symbols is or at least should
be automatable.

_mod_foo bothers me a lot more than whether people write foo.FooFoo or
just FooFoo.

My inclination is towards a policy of:

 * use whatever is clearest in the code as you write it
 * avoid gratuituous changes
 * perhaps identify some particular modules (errors, ui, ...?) that
should usually be imported as modules
 * if there are hard-to-fix limits on what you can do with lazyimport,
document them

I'm interested in what others think though.
--
Martin

Martin Pool wrote:
[...]
> My inclination is towards a policy of:
>
> * use whatever is clearest in the code as you write it
> * avoid gratuituous changes
> * perhaps identify some particular modules (errors, ui, ...?) that
> should usually be imported as modules
> * if there are hard-to-fix limits on what you can do with lazyimport,
> document them
>
> I'm interested in what others think though.

That matches my inclinations pretty much perfectly.

>>>>> Robert Collins <email address hidden> writes:

    > I'm not going to vote on this, but I would like to say that I'm still
    > totally unconvinced. I think that this is a flawed argument and an
    > accomodation to other design issues.

    > I've got a few point by point notes below.

    > + The bad points of the ``from <module> import symbol`` style includes:
    > +
    > + * The symbol list for each import is harder to keep up to date.

    > This isn't a compelling point: if optimising for that is a concern,
    > best to do 'from sys import modules' and access everything via
    > modules['bzrlib.foo.bar.baz'] - the ultimate in maintenance.

Invoking excessive and irrelevant examples is not really a good way to
find what we want to do.

    > Really this is a tradeoff - its more repetition, more typing,
    > and code gets more verbose.

I think I addressed that, I don't care if the code is harder to write if
it makes it easier to read.

Having a rule that tells me: this function is local to the module you
can search for it, this function is imported from this module, go there
for details.

And also, at the top-level, these are the modules we're are using
(preferably a hosrt list) instead of pages of symbols I won't be able to
grasp.

    > Codifying one particular value on the tradeoff as 'right' is IMO a
    > mistake.

Well, a symbol is defined in a module, importing it create an alias,
another definition of the symbol, my proposal is about sticking to *one*
definition of the symbol. What's wrong with that ?

    > + * The imported symbols can be imported by other modules without realizing
    > + the aliasing that is occurring.

    > This can happen if you use imports at all. 'from bzrlib.tests import
    > bzrlib as foo'.

Yeah, there are infinite variation to create aliases, that's the
idea. But this example is:
- rather convoluted,

- violating the spirit of the rule I'm proposing: import modules by
  their name *unless* it conflicts with variable names (and even there I
  would be glad to push for the variable or module renaming to make
  symbols unambiguous, but I refrain ;)

    > The standard convention in the python world is to set __all__ to
    > declare what should be exported, and you can lint to warn.

This can be done too and may be a better solution if it can be enforced
(which I doubt, both in the technical and social meanings) but this
sounds like a significant amount of work.

    > If such aliasing is a concern. (I don't see why it matters - whats
    > the functional harm?).

Proliferation of aliases makes the code harder to read and to maintain.

    > + * It forbids overriding the imported symbol for test purposes.

    > This is a dangerous testing approach anyway: changing the global
    > symbol table is a Big Hammer and should be avoided as much as
    > possible.

What global symbol table ? We're talking about the *module* symbol table
and whether we want to modify it once at the module level or at every
call site.

And yeah, badly used it can be dangerous, so what ? There are many
things that are dangerous in testing and we use them with
care. overrideAttr is used roughly 100 times...

Vincent Ladeuil wrote:
[...]
> > The standard convention in the python world is to set __all__ to
> > declare what should be exported, and you can lint to warn.
>
> This can be done too and may be a better solution if it can be enforced
> (which I doubt, both in the technical and social meanings) but this
> sounds like a significant amount of work.

IIRC, pylint can usually warn about violations of this (although in our
case we'd probably need to teach it about lazy_imports).

[...]
> > + * It makes it easier to move code from one module to another. You don't
> > + need to sort out which symbols need to be copied across the new module
> > + or deleted in the old, you only have to care about modules (once).
>
> > If thats an issue, pyflakes or other things can sort that out
> > pretty darn quickly.
>
> It hasn't so far...

Yes it does, I use pyflakes for exactly this already. Move some code
between files, run pyflakes on both: it will tell you which imports are
unused in one, and which are names do not yet exist (and so need to be
imported) in the other.

-Andrew.

>>>>> Martin Pool <email address hidden> writes:

    > I'm glad you put this up, because I'd rather have the conversation
    > once and decide what we agree upon, than to have it as a side
    > track from some other change or even worse to have them flip too
    > and fro.

    > typo 'drawbaks'.

    > I agree with Robert that this tends to make the code longer and all
    > else being equal we should resist that.

    > If this breaks lazy imports then ideally we would be fixing lazy
    > imports, or at worst documenting that things must be given in this
    > style specifically when they are lazily imported. I think lazy
    > imports are kind of a useful kludge anyhow, and not something that
    > should shift our idea of what is clearest.

Right, so why isn't prefixing a symbol by its module clearer than making
all symbols used state from which module they are coming ?

That's the whole idea behing hierarchical name spaces to me.

I feel that we are not clear there, we don't use 'from module import *'
right ? Why ? Isn't it because it's confusing ?

Then why do we stop here ?

And why so many exceptions to the rule ?

    > One interesting case is 'ui.ui_factory' where we really do want to be
    > able to change the variable later.

This is related yes, and believe ot not, we consider all our functions
as exiting only once so why don't we refer to them in an unambiguous way ?

    > Perhaps this indicates that pointing to it through a global
    > variable is not ideal.

Right, globals are bad, unless they are used with care, and we do that,
I don't think we *abuse* globals in bzrlib do we ? The rule is still to
avoid globals. The meta-rule is still to respect rules unless you
document your exceptions, which we do for ui.ui_factory.

    > I think for things like 'errors.' where you do want to pull in
    > multiple classes it can be annoying to manually update the symbol
    > list.

What is more important for me here is that when I read 'errors.' I don't
even have to think about it, it's an exception used by several modules
(otherwise it should be defined locally and other modules will use
<module>.exception making it clear that the exception is specific to the
module which also carry some information).

    > It seems like updating or checking the symbols is or at least should
    > be automatable.

    > _mod_foo bothers me a lot more than whether people write foo.FooFoo or
    > just FooFoo.

Indeed. I would prefer to *not* use this workaround by either renaming
the modules to leave the variable name free or use other variable names

The most notable cases are transport and revision and using them as
variable names can generally be avoided by using 't' or 'r' or whatever
abbreviation without loss of clarity. If the code use a *single*
transport it's generally obvious it's a transport when the variable is
initialized and if the code use this variable across multiple *pages*,
we have a different problem to address anyway. If they are multiple
transport objects, the code can generally makes it clear which is which
by *not* using transport either.

    > My inclination is towards a policy of:

    > * use whatever is clearest in t...

>>>>> Andrew Bennetts <email address hidden> writes:

<snip/>
    >>
    >> > If thats an issue, pyflakes or other things can sort that out
    >> > pretty darn quickly.
    >>
    >> It hasn't so far...

    > Yes it does,

I meant it isn't documented nor inforced.

It's on my TODO list for babune though but still pretty low there.

    > I use pyflakes for exactly this already. Move some code between
    > files, run pyflakes on both: it will tell you which imports are
    > unused in one, and which are names do not yet exist (and so need
    > to be imported) in the other.

And what are the results across bzrlib then ?

Do we progress or do we regress over time ?

Vincent Ladeuil wrote:
[...]
> > * use whatever is clearest in the code as you write it
>
> My point is that not prefixing by the module name doesn't make it clear,
> can someone explain why they feel it's clearer to not use it ?

Most of our classes (and other module-global attributes) have names that
are unique, or very close to unique, across bzrlib. Having “bzrdir.” in
front of “BzrDir” doesn't make it clearer, and clutter can harm
readability. This is especially true for longer names, e.g.
“GCRepositoryPackCollection” is quite long enough (and distinct enough)
without adding “groupcompress_repo.” to it.

I'm not saying there a module prefix is always less clear, but equally
it seems obvious to me that it won't always be clearer.

Vincent Ladeuil wrote:
[...]
> > I use pyflakes for exactly this already. Move some code between
> > files, run pyflakes on both: it will tell you which imports are
> > unused in one, and which are names do not yet exist (and so need
> > to be imported) in the other.
>
> And what are the results across bzrlib then ?

Most modules have a few unused imports. Although the cost (in terms of
source size and runtime) of a redundant “import os” and the like is
pretty trivial.

> Do we progress or do we regress over time ?

In code I touch, it progresses ;)

I haven't kept any historical stats, but I guess we could dig through
our revision history...

Vincent Ladeuil wrote:
[...]
> > * avoid gratuituous changes
>
> Sure, we should not do that, in the case at point though, the expected
> benefit is to get a coherent and clearer code base and I think it's
> worth spending time getting there. Or failing that, ensure that
> modifications go into this direction and stating so.

Also, I can't said I find our current idioms for importing names to be a
significant issue for the clarity of our code. After all, regardless of
whether an expression uses “module.Thing” or just “Thing”, ctags
generally does a good job of jumping straight to it.

The biggest problem I encounter are deep inheritance hierarchies or
complex compositions of objects that delegate in a deep chain (or
combinations of both) — then finding the relevant definition of
“add_lines”, “make_branch” or “get_parent_map” or whatever is tedious
and frustrating. I don't have any good ideas about improving that
situation (beyond trying to keep complexity down as much as possible),
but compared to that problem looking up which module “Thing” was
imported from is... almost trivial.

[...]
> > * perhaps identify some particular modules (errors, ui, ...?) that
> > should usually be imported as modules
>
> I'd be fine with this restricted goal if it was clearly defined, so
> let's make sure we understand why we disagree about why this rule is
> good or bad and in which cases.

In the case of errors: because it's common to need many, many names from
it (and often each name only once or twice per module), so it is usually
noticeably more convenient to import errors once, rather than import 15
errors individually. There are probably few modules like this.
bzrlib.tests is the other that springs to mind.

In the case of ui: because there's a module attribute that may change
(and as Martin says, that's possibly a bad reason). There are almost
certainly very few modules like this, and we should be striving to keep
them to a minimum.

> Most of our classes (and other module-global attributes) have names that
> are unique, or very close to unique, across bzrlib. Having “bzrdir.” in
> front of “BzrDir” doesn't make it clearer, and clutter can harm
> readability. This is especially true for longer names, e.g.
> “GCRepositoryPackCollection” is quite long enough (and distinct enough)
> without adding “groupcompress_repo.” to it.
>
> I'm not saying there a module prefix is always less clear, but equally
> it seems obvious to me that it won't always be clearer.

Ha, at least we're going into cases easier to discuss.

Right, bzrdir.BzrDir isn't clearer than BzrDir. In this case 'Bzr' is clearly unambiguous.

Now bzrlib.repository err, bzrlib.repofmt.groupcompress_repo.GCRepositoryPackCollection
- is not under bzrlib/repository, why ? To shorten the path ?
- why adding _repo to groupcompress
- why prefixing again by GCRepository

How about bzrlib.repo.gc.PackCollection which will be used as gc.PackCollection ?

I'm not trying to force a rule that nobody likes, but I think that having *no* rules doesn't work either nor having too many rules.

So when the symbol name already embed most of its module path, adding the module path (<ducks> duh, will at least make this more obvious </ducks>) seems redundant but we're not doing this everywhere either.

Is this another aspect of the same problem ? Is it good ? Bad ?
What's the rule here ?

I kind of feel that if we can come with good answers about how we want to manage the bzrlib namespace (this proposal being only a tiny part of it) several problems may become more apparent and easier to address:
- the startup time and the lazy importing,
- some configuration of bzrlib itself,
- navigation and documentation,
- explaining to contributors how they should write new code.

I'm also afraid that relying on good will only will lead to the same status-quo about spurious spaces: we recommend avoiding them, regularly tell contributors to fix some, have tests to block them if needed but... nothing (almost) happens (i.e. the numbers ouput by bt.test_source is almost constant but slowly *increasing*):

for 2.0.6:

 There are 294 lines with trailing white space in 96 files.
 There are 2006 lines longer than 79 characters in 316 files.

for 2.3b1:

  There are 563 lines with trailing white space in 167 files.
  There are 2066 lines longer than 79 characters in 339 files.

On Thu, Sep 23, 2010 at 9:59 PM, Vincent Ladeuil <email address hidden> wrote:
> I'm also afraid that relying on good will only will lead to the same status-quo about spurious spaces: we recommend avoiding them, regularly tell contributors to fix some, have tests to block them if needed but... nothing (almost) happens (i.e. the numbers ouput by bt.test_source is almost constant but slowly *increasing*):

yes, but really, as a metric for code base quality, what does this really mean?

Many years ago I had a period where I cared a great deal about such
things; these days I consider them valueless: a distraction from what
really matters.

-Rob

On 23 September 2010 19:59, Vincent Ladeuil <email address hidden> wrote:
>> Most of our classes (and other module-global attributes) have names that
>> are unique, or very close to unique, across bzrlib.  Having “bzrdir.” in
>> front of “BzrDir” doesn't make it clearer, and clutter can harm
>> readability.  This is especially true for longer names, e.g.
>> “GCRepositoryPackCollection” is quite long enough (and distinct enough)
>> without adding “groupcompress_repo.” to it.
>>
>> I'm not saying there a module prefix is always less clear, but equally
>> it seems obvious to me that it won't always be clearer.
>
> Ha, at least we're going into cases easier to discuss.
>
> Right, bzrdir.BzrDir isn't clearer than BzrDir. In this case 'Bzr' is clearly unambiguous.

Right.

> Now bzrlib.repository err, bzrlib.repofmt.groupcompress_repo.GCRepositoryPackCollection
> - is not under bzrlib/repository, why ? To shorten the path ?
> - why adding _repo to groupcompress
> - why prefixing again by GCRepository

I think that's a good example of a fairly poor full name. I would be
more interested in moving/renaming it than in calling it
groupcompress_repo.GCRepositoryPackCollection or not.
Indeed that dotted name is so unwieldy that I think it demonstrates
that we should not have a general requirement for module.symbol.

> How about bzrlib.repo.gc.PackCollection which will be used as gc.PackCollection ?

Good question. I think it is worth generally making the class names
unique in their own right, rather than relying on the module name to
disambiguate them. I would _not_ want to have
repofmt.weave.Repository, plugins.svn.Repository,
repofmt.gc.Repository, etc. In other languages that might work but I
think not in Python:

  * reprs and similar things tend to print just the class name by
default, which does not include the qualifiers; if these classes ever
turn up in exceptions, test failures, or logs they're going to be hard
to read; I have wasted time before debugging built-in modules that
declare something called eg Exception that's different from the
builtin Exception

 * ctags, grep, editor search etc tend to work or to be most naturally
used word-at-a-time

 * once you start doing this you lock all code that uses these classes
into using qualified names or risking confusion

I wouldn't insist all names are unique across the codebase but it is a
very good idea.

Without studying it in detail I'd say that class should probably be
something like bzrlib.groupcompress.GroupCompressPackCollection.

> I'm not trying to force a rule that nobody likes, but I think that having *no* rules doesn't work either nor having too many rules.

I think we're having a good constructive conversation to work out what
the rules should be. There's no need to duck; no one will throw
things for you raising this.

> So when the symbol name already embed most of its module path, adding the module path (<ducks> duh, will at least make this more obvious </ducks>) seems redundant but we're not doing this everywhere either.
>
> Is this another aspect of the same problem ? Is it good ? Bad ?
> What's the rule here ?
>
> I kind of feel that if we can come with good answers about how w...