[PATCH 1 of 4] help: adding a new help topic about extensions

[Cedric Duval]

Hi Dirkjan,

Dirkjan Ochtman wrote:
> 2009/6/20 Cédric Duval <cedricduval at free.fr>:
> > +# borrowed from pydoc
> > +def pathdirs():
> > +    '''Convert sys.path into a list of absolute, existing, unique paths.'''

> Your function comment should be put into the docstring. Doesn't make
> much sense to have two separate places to comment on the function.

The former is more a licensing/attribution matter, while the latter is a
functionnal comment, that's why I separated them. But sure, I can regroup.

> > +def additionalextensions():

> Don't like the name too much. Maybe disabled()?

I'm not pleased with the long names either, I was only wary of using too
generic names, I am otherwise happy to switch for disabled().

> > +    for dir in filter(os.path.isdir,
> > +                      (os.path.join(pd, 'hgext') for pd in pathdirs())):

> Please don't use filter. Use a list comprehension instead. For best
> results, put it on a separate line so it doesn't wrap.

> Might make more sense to leverage os.walk() instead of having a nested
> loop here.

Ok, I'll have a look at it.

> > +def topicextensions():

> This should probably just be called extensions().

I'd like to, but it causes a name clash

  File "mercurial/help.py", line 103, in enabledextensions
    enabled = list(extensions.extensions())
  AttributeError: 'function' object has no attribute 'extensions'

> > +        doc += _('\nnon-enabled extensions:\n\n')

> non-enabled -> disabled.

Ok. I told timeless about my rationale for not choosing 'disabled' (for
a reason of active vs passive semantics), but if you both think there is
no problem with it, I will conclude that it is certainly a wrong
impression of mine, as a non-native speaker. :)

> Actually, it may make more sense to put disabled() and enabled() in
> mercurial.extensions. It's not really a help thing, after all.

Yes, probably. I'll do that in a separate patch.

> > +    if enabled:

> It'd be nicer to return if not enabled, saves on the indentation later.

Indeed, that's how I always do in C. But a lot of python code I had seen
seemed to resolve around a single point of return, so I sticked to that.
Happy to change. :)

> > +def extensionslisting(header, exts, maxlength):

> Please rename... listexts()?

Ok.

> > # User Cédric Duval <cedricduval at free.fr>
> > # Date 1245524150 -7200
> > # Node ID 7497bc3833c03c79bc410eb7009d5407d09b25e9
> > # Parent  34c094dfd6a93a402269a5ee8e7aa0840d5365b8
> > help: expand the extensions topic

> Looks good, but I think this should either come before the cset
> introducing the extensions topic or should be merged with it.

As far as I know Matt has already queued the patches, so it might be
too late to amend them in-place. I'll post patches on top of those.

Thanks a lot for the reviews!
-- 
Cédric