On 1/6/2014, 1:22 PM, Axel Hecht wrote:
Hi,
two points of caution:
In the little version control archaeology I do, I hit "breaks blame for
no good reason" pretty often already. I'd not underestimate the cost for
the project of doing changes just for the sake of changes.
I have very rarely run into a blame session where you find what you're
looking for by just running blame once. Most of the time you need to
reblame on older revisions several times, and whitespace changes will
add at most one step if entire files are changed in single patches.
That being said, both hg and git support options to ignore whitespace
changes during a blame, so this issue is moot in practice (hgweb doesn't
support it, but it's a terrible blame tool anyway.)
Tools don't get code right. I've seen various changes where tools
reformat code they don't understand, and break it. Trailing whitespace
is significant in some of our file formats, for example.
They're insignificant in C, C++, JS and Java.
Cheers,
Ehsan
On 1/6/14 6:55 PM, Martin Thomson wrote:
I think that this is a good start, but it doesn’t go quite far enough.
Part of the problem with a policy that requires people to avoid
reformatting of stuff they don’t touch is that it propagates
formatting divergence. Sometimes because it’s better to conform to
the style immediately adjacent the changed, but more so because it
prevents the use of tools that reformat entire files (here, I’m not
talking about things like names, but more whitespace conventions).
For whitespace at least, I think that we need to do the following:
1. pick a style
I really, really don’t care what this is. I’m thinking that we pick
whatever people think that the current style is and give folks a fixed
period to debate changes.
2. create some tools
These tools should help people conform to the style.
Primarily, what is needed is a tool with appropriate configuration
that runs on the command line — e.g., mach reformat … clang-format is
looking like a good candidate for C/C++, it just needs a
configuration. For JavaScript, I’ve used the python js-beautify, but
it’s pretty light on configuration options, it might need some hacking
to make it better.
Ideally though, there should be set of configuration files for common
editors. I’m certain there are plenty out there already. Let’s bring
them all together (attaching the files
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style
might be enough).
3. reformat everything
Take the command line tool and run it over all the code. I realise
that this is the contentious part, but you don’t get the real benefits
until you do this.
Once you do this, it’s safe to reformat files at any time without
messing with parts of files that you haven’t touched. This is
important because many tools only reformat entire files.
As for `hg blame` and related tools, I have a workaround for that.
I’ve knocked together a tool that takes a file and a reformatter
command line, and churns out a series of patches that retain blame:
https://github.com/martinthomson/blame-bridge
The patch bitrot problem is not easy to work around, but that will
depend on how closely the affected files already conformed to the
style guide.
4. enforce compliance
This is probably a step for the future, but if there was - for example
- a commit bot that waited for a clean build+test run, adding a format
check to that run would allow the bot to block patches that screwed up
the formatting of files.
—Martin
On 2014-01-05, at 18:34, Nicholas Nethercote <n.netherc...@gmail.com>
wrote:
We've had some recent discussions about code style. I have a propasal
For the purpose of this proposal I will assume that there is
consensus on the
following ideas.
- Having multiple code styles is bad.
- Therefore, reducing the number of code styles in our code is a win
(though
there are some caveats relating to how we get to that state, which
I discuss
below).
- The standard Mozilla style is good enough. (It's not perfect, and
it should
continue to evolve, but if you have any pet peeves please mention
them in a
different thread to this one.)
With these ideas in mind, a goal is clear: convert non-Mozilla-style
code to
Mozilla-style code, within reason.
There are two notions that block this goal.
- Our rule of thumb is to follow existing style in a file. From the
style
guide:
"The following norms should be followed for new code, and for Tower
of Babel
code that needs cleanup. For existing code, use the prevailing
style in a
file or module, or ask the owner if you are on someone else's turf
and it's
not clear what style to use."
This implies that large-scale changes to convert existing code to
standard
style are discouraged. (I'd be interested to hear if people think this
implication is incorrect, though in my experience it is not.)
I propose that we officially remove this implicit discouragement,
and even
encourage changes that convert non-Mozilla-style code to
Mozilla-style (with
some exceptions; see below). When modifying badly-styled code,
following
existing style is still probably best.
However, large-scale style fixes have the following downsides.
- They complicate |hg blame|, but plenty of existing refactorings
(e.g.
removing old types) have done likewise, and these are bearable if
they
aren't too common. Therefore, style conversions should do entire
files in
a single patch, where possible, and such patches should not make any
non-style changes. (However, to ease reviewing, it might be worth
putting fixes to separate style problems in separate patches.
E.g. all
indentation fixes could be in one patch, separate from other
changes.
These would be combined before landing. See bug 956199 for an
example.)
- They can bitrot patches. This is hard to avoid.
However, I imagine changes would happen in a piecemeal fashion,
e.g. one
module or directory at a time, or even one file at a time. (Again,
see bug
956199 for an example.) A gigantic change-all-the-code patch seems
unrealistic.
- There is an semi-official policy that the owner of a module can
dictate its
style. Examples: SpiderMonkey, Storage, MFBT.
There appears to be no good reason for this and I propose we remove
it.
Possibly with the exception of SpiderMonkey (and XPConnect?), due
to it being
an old and large module with its own well-established style.
Also, we probably shouldn't change the style of imported
third-party code;
even if we aren't tracking upstream, we might still want to trade
patches.
(Indeed, it might even be worth having some kind of marking at the
top of
files to indicate this, a bit like a modeline?)
Finally, this is a proposal only to reduce the number of styles in our
codebase. There are other ideas floating around, such as using
automated tools
to enforce consistency, but I consider them orthogonal to or
follow-ups/refinements of this proposal -- nothing can happen unless
we agree
on a direction (fewer styles!) and a way to move in that direction
(non-trivial
style changes are ok!)
Nick
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
