[RFC] CommitTemplate (was Re: [ANN bzr-vim] syntax and ftplugin for bzr_log.* files)
John Arbash Meinel
john at arbash-meinel.com
Fri Jul 28 16:15:22 BST 2006
Adeodato Simó wrote:
>> Bundle attached.
>
> Yay.
>
>
The concept of a customizable commit template has promise. However,
there are quite a few code style issues with what you have submitted.
As a general statement, we follow PEP8 (which I recommend reading)
http://www.python.org/dev/peps/pep-0008/
We don't follow it 100%, though we are close. (And Aaron, PEP8 actually
says you should use 2 spaces after periods. I haven't done that in a
long time, since LaTeX (and Word) always take care of that for me, but I
can try to remember.)
The other major comment, is that we are committed to maintain backwards
compatibility for a period of time. (at least 6 months, and at least 1
release with the code being deprecated, IIRC). So we can deprecate, but
can't remove public functions, or change their signature in a way that
old callers would break (such as arbitrary parameter name changes, or
removing existing parameters).
v- As Aaron said, the first line should be a one line summary (kind of
like the PEP8 docstring.
The biggest reason for that is 'bzr log --line' will only print the
first line. Same thing with 'bzr status' and pending merges.
So whatever template you use, it should encourage people to start with a
summary line.
Arguably you could even have the commit template start with:
Summary:
-----
other info here
And then strip out 'Summary:' when it reads in the final text.
> # message:
> # bzrlib/commit_template.py:
> # - New file.
> #
> # Generalize the creation of templates for commit files with a
> # CommitTemplate class, and register them the way log.py does with
> # LogFormatter.
> #
...
v- Aaron mentioned that Copyright should have a solid block of '#' on
the left.
> +# Copyright (C) 2006 by Canonical Ltd
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +
...
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +
v- PEP8 says 2 blank lines between top level code sections. So you need
another blank line between the opening docstring and the first class.
> +"""Commit file template support."""
> +
> +class CommitTemplate(object):
> + """Abstract class to prepare a commit template."""
> +
v- I don't think we use lined up '=' anywhere else in bzrlib. I don't
know that PEP8 specifically says this, but it might.
So all these should be changed to:
self.working_tree = working_tree
self.specific_files = specific_files
Especially since you aren't even consistent within the same code block.
(I also think we want to use the tight equals, because otherwise when
you add a new entry, you may need to re-indent all the other lines to
make them all line up. And that adds unnecessary lines in the diff,
which just make it harder to review).
> + def __init__(self, working_tree, specific_files):
> + self.working_tree = working_tree
> + self.specific_files = specific_files
> +
> + self.template = self._template()
> + self.comment = self._comment()
> + self.ignoreline = self._ignoreline()
> +
> + def _template(self):
> + return '\n'
> +
> + def _comment(self):
> + return '\n'
> +
> + def _ignoreline(self):
> + return "%(bar)s %(msg)s %(bar)s" % { 'bar' : '-' * 14,
> + 'msg' : 'This line and the following will be ignored' }
^- Also here. PEP8 doesn't have extra whitespace, so the dict is
properly formatted as:
% {'bar':'-'*14,
'msg':'This line and the following line will be ignored'}
I think it is probably cleaner to declare the dict on another line, and
just use it.
> +
> +
> +class DefaultCommitTemplate(CommitTemplate):
> + def _comment(self):
> + from StringIO import StringIO # must be unicode-safe
> + from bzrlib.status import show_tree_status
> +
> + status_tmp = StringIO()
> + show_tree_status(self.working_tree, specific_files=self.specific_files,
> + to_file=status_tmp)
> +
> + return status_tmp.getvalue()
^- If you have to use StringIO.StringIO, then you will have a bug.
Because writing to the file will do the wrong thing (I believe).
I think you need a codec wrapped file, that is wrapped in
bzrlib.user_encoding() so that when the file contents are generated,
they will be in the right user encoding.
> +
> +
> +class DiffCommitTemplate(DefaultCommitTemplate):
> + def _comment(self):
> + comment = DefaultCommitTemplate._comment(self)
> + # TODO implement for real
> + comment += "\n# START DIFF"
> + return comment
> +
> +
> +# TODO This is copied from log.py, maybe refactor?
> +COMMIT_TEMPLATES = {
> + 'default': DefaultCommitTemplate,
> + 'diff': DiffCommitTemplate,
> + # Remember to update config.__doc__ when adding a template
> +}
I greatly prefer private variables for factory dicts to all CAPS ones.
So:
_commit_template = {}
That also makes it clear that people shouldn't be directly modifying the
dict, instead they should be using the register_* function.
I don't really prefer *args, **kwargs functions when you are going to
have a restricted set anyway. (The classes themselves need to use the
same api).
The reason log used *args,**kwargs is because the class existed before
the function. Since we are starting from scratch, we should do it right.
> +
> +def commit_template(name, *args, **kwargs):
> + """Construct a CommitTemplate from arguments.
> +
> + name:
> + Name of the CommitTemplate to construct; currently 'default' and
> + 'diff' are supported.
> + """
> + from bzrlib.errors import BzrCommandError
> + try:
> + return COMMIT_TEMPLATES[name](*args, **kwargs)
> + except KeyError:
> + raise BzrCommandError("unknown commit template: %r" % name)
> +
> +def register_commit_template(name, commit_template):
> + COMMIT_TEMPLATES[name] = commit_template
> +
> +def available_commit_templates():
> + return COMMIT_TEMPLATES.keys()
^- you need extra blank lines between all of these functions.
>
> === modified file bzrlib/builtins.py
> --- bzrlib/builtins.py
> +++ bzrlib/builtins.py
> @@ -1678,6 +1678,8 @@
>
> # XXX: verbose currently does nothing
>
> + from bzrlib.commit_template import available_commit_templates
> +
^- Importing at class scope is *really* *really* ugly.
Also, we generally prefer to create a helper function, so that we get an
instance/class/whatever as the run argument, rather than the plain string.
Something like:
def get_commit_template_class(name):
return _commit_templates[name]
Option('template', type=get_commit_template_class,
help='blah blah')
Then in the 'run' command, you have a class, which you then instantiate
with the right arguments.
> takes_args = ['selected*']
> takes_options = ['message', 'verbose',
> Option('unchanged',
> @@ -1688,6 +1690,12 @@
> Option('strict',
> help="refuse to commit if there are unknown "
> "files in the working tree."),
> + Option('template', type=str,
> + argname='templat',
> + help="template format to use for commit files. "
> + "Possible values are: %s." % (', '.join(
> + sorted(available_commit_templates())))
> + ),
> Option('local',
> help="perform a local only commit in a bound "
> "branch. Such commits are not pushed to "
> @@ -1698,12 +1706,12 @@
> aliases = ['ci', 'checkin']
>
> def run(self, message=None, file=None, verbose=True, selected_list=None,
> - unchanged=False, strict=False, local=False):
> + unchanged=False, strict=False, template=None, local=False):
> from bzrlib.commit import (NullCommitReporter, ReportCommitToLog)
> from bzrlib.errors import (PointlessCommit, ConflictsInTree,
> StrictCommitFailed)
> - from bzrlib.msgeditor import edit_commit_message, \
> - make_commit_message_template
> + from bzrlib.msgeditor import edit_commit_message
> + from bzrlib.commit_template import commit_template
> from tempfile import TemporaryFile
^- As Aaron mentioned, we need to leave parameters in order whenever
possible.
>
> # TODO: Need a blackbox test for invoking the external editor; may be
> @@ -1724,7 +1732,9 @@
> if local and not tree.branch.get_bound_location():
> raise errors.LocalRequiresBoundBranch()
> if message is None and not file:
> - template = make_commit_message_template(tree, selected_list)
> + if (template == None):
> + template = tree.branch.get_config().commit_template()
> + template = commit_template(template, tree, selected_list)
> message = edit_commit_message(template)
> if message is None:
> raise BzrCommandError("please specify a commit message"
I do like user-configurable / per-branch templates.
>
> === modified file bzrlib/config.py
> --- bzrlib/config.py
> +++ bzrlib/config.py
> @@ -28,6 +28,7 @@
> create_signatures=always|never|when-required(default)
> gpg_signing_command=name-of-program
> log_format=name-of-format
> +commit_template=name-of-template
>
> in locations.conf, you specify the url of a branch and options for it.
> Wildcards may be used - * and ? as normal in shell completion. Options
> @@ -51,6 +52,9 @@
> branch is configured to require them.
> log_format - This options set the default log format. Options are long,
> short, line, or a plugin can register new formats
> +commit_template - this option sets the default template for commit files.
> + Possible values are default, diff, or a plugin can register
> + new templates.
>
> In bazaar.conf you can also define aliases in the ALIASES sections, example
>
> @@ -144,6 +148,17 @@
> """See log_format()."""
> return None
>
> + def commit_template(self):
> + """What commit template class should be used"""
> + result = self._commit_template()
> + if result is None:
> + result = "default"
> + return result
> +
> + def _commit_template(self):
> + """See commit_template()."""
> + return None
> +
> def __init__(self):
> super(Config, self).__init__()
>
> @@ -286,6 +301,10 @@
> """See Config.log_format."""
> return self._get_user_option('log_format')
>
> + def _commit_template(self):
> + """See Config.commit_template."""
> + return self._get_user_option('commit_template')
> +
> def __init__(self, get_filename):
> super(IniBasedConfig, self).__init__()
> self._get_filename = get_filename
> @@ -560,6 +579,10 @@
> """See Config.log_format."""
> return self._get_best_value('_log_format')
>
> + def _commit_template(self):
> + """See Config.commit_template."""
> + return self._get_best_value('_commit_template')
> +
>
> def ensure_config_dir_exists(path=None):
> """Make sure a configuration directory exists.
>
> === modified file bzrlib/msgeditor.py
> --- bzrlib/msgeditor.py
> +++ bzrlib/msgeditor.py
> @@ -25,6 +25,7 @@
>
> import bzrlib
> import bzrlib.config as config
> +import bzrlib.commit_template as c_t
^- you can do:
from bzrlib import commit_template
But please don't use variables like 'c_t'. They are very unmaintainable.
> from bzrlib.errors import BzrError
> from bzrlib.trace import warning, mutter
>
> @@ -74,21 +75,16 @@
> " - $BZR_EDITOR\n - editor=/some/path in %s\n - $EDITOR" % \
> config.config_filename())
>
> -
> -DEFAULT_IGNORE_LINE = "%(bar)s %(msg)s %(bar)s" % \
> - { 'bar' : '-' * 14, 'msg' : 'This line and the following will be ignored' }
> -
> -
> -def edit_commit_message(infotext, ignoreline=DEFAULT_IGNORE_LINE):
> +_marker = object()
> +
> +def edit_commit_message(commit_template, ignoreline=_marker):
> """Let the user edit a commit message in a temp file.
>
> This is run if they don't give a message or
> message-containing file on the command line.
>
> - infotext:
> - Text to be displayed at bottom of message for
> - the user's reference; currently similar to
> - 'bzr status'.
> + commit_template:
> + A CommitTemplate object to include in the file.
> """
> import tempfile
>
> @@ -96,11 +92,24 @@
> try:
> tmp_fileno, msgfilename = tempfile.mkstemp(prefix='bzr_log.', dir=u'.')
> msgfile = os.close(tmp_fileno)
> - if infotext is not None and infotext != "":
> +
> + if commit_template is not None:
> hasinfo = True
> msgfile = file(msgfilename, "w")
> - msgfile.write("\n\n%s\n\n%s" % (ignoreline,
> - infotext.encode(bzrlib.user_encoding, 'replace')))
> + if not isinstance(commit_template, c_t.CommitTemplate):
> + # assume previous API (string)
> + template = "\n"
> + comment = commit_template
> + if ignoreline == _marker:
> + ignoreline = c_t.DefaultCommitTemplate.ignoreline
> + else:
> + template = commit_template.template
> + comment = commit_template.comment
> + ignoreline = commit_template.ignoreline
> +
> + parts = map(lambda s: s.encode(bzrlib.user_encoding, 'replace'),
> + [ template, ignoreline, comment ])
> + msgfile.write("%s\n%s\n%s\n" % tuple(parts))
> msgfile.close()
> else:
> hasinfo = False
> @@ -133,6 +142,9 @@
> return ""
> # delete empty lines at the end
> del msg[lastline:]
> + # maybe there was a template left unmodified
> + if "".join(msg) == template:
> + return ""
> # add a newline at the end, if needed
> if not msg[-1].endswith("\n"):
> return "%s%s" % ("".join(msg), "\n")
> @@ -145,23 +157,3 @@
> os.unlink(msgfilename)
> except IOError, e:
> warning("failed to unlink %s: %s; ignored", msgfilename, e)
v- this was a public api, so we can't just remove it. You need to
provide a compatibility interface to the new way of doing it.
> -
> -
> -def make_commit_message_template(working_tree, specific_files):
> - """Prepare a template file for a commit into a branch.
> -
> - Returns a unicode string containing the template.
> - """
> - # TODO: Should probably be given the WorkingTree not the branch
> - #
> - # TODO: make provision for this to be overridden or modified by a hook
> - #
> - # TODO: Rather than running the status command, should prepare a draft of
> - # the revision to be committed, then pause and ask the user to
> - # confirm/write a message.
> - from StringIO import StringIO # must be unicode-safe
> - from bzrlib.status import show_tree_status
> - status_tmp = StringIO()
> - show_tree_status(working_tree, specific_files=specific_files,
> - to_file=status_tmp)
> - return status_tmp.getvalue()
I hope that isn't too much. I think there should be a way to
enable/disable certain features of a commit template.
I think you could do it as:
bzr commit --template-options=diff,merges,foo,baz,bar
And then they can either use an alias, or a per-branch/global
configuration to set the default.
John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060728/6048110e/attachment.pgp
More information about the bazaar
mailing list