Simple tasks for students
On 7/19/2017 3:56 PM, Ben Bucksch via Maildev wrote:
For the other points , could you please give a concrete example which
existing code you would format how? Details matter here, we care down
to whitespace. This avoids that they reformat 20 classes and then have
to change them all.
Yes of course that would be part of the process, and I expect there to
be controversies there. The first step is to understand what the issues
are likely to be, and agree on the desired style going forward.
I tested today running eslint on some of the /mail files, using the
eslint rules from the Calendar project. A typical file has 99+ errors.
Most of those are just a few style differences, which we are going to
have to think about.
The main policy question is, do we want to provide linting using eslint
on other directories in comm-central, including accepting all of the
likely whitespace changes that will result from such a cleanup? I
believe I am hearing a yes on this. I'm not asking you to approve
specific rules, or just accept whatever I give you.
On a related issue, even the Calendar files are showing a few linting
errors. The Mozilla linting page claims eslint: "runs on treeherder for
every check-in." Can anyone verify that? That is, is there an automatic
test that runs that will catch linting errors once enabled? If so, why
is Calendar still having issues after last year's fix?
But this is a bit of a bikeshedding issue here, so I would hope that
most issues can be resolved with agreements between Magnus and I without
having to argue here over every space.
:rkent
On Wed, Jul 19, 2017 at 6:28 PM R Kent James via Maildev <
maildev@lists.thunderbird.net> wrote:
On a related issue, even the Calendar files are showing a few linting
errors. The Mozilla linting page claims eslint: "runs on treeherder for
every check-in." Can anyone verify that? That is, is there an automatic
test that runs that will catch linting errors once enabled? If so, why
is Calendar still having issues after last year's fix?
I bet that thunderbirds build system isn't invoking eslint. If you file a
bug and assign me, I can look into it.
On 7/19/2017 5:32 PM, Tom Prince via Maildev wrote:
On Wed, Jul 19, 2017 at 6:28 PM R Kent James via Maildev
<maildev@lists.thunderbird.net mailto:maildev@lists.thunderbird.net>
wrote:
On a related issue, even the Calendar files are showing a few linting
errors. The Mozilla linting page claims eslint: "runs on
treeherder for
every check-in." Can anyone verify that? That is, is there an
automatic
test that runs that will catch linting errors once enabled? If so, why
is Calendar still having issues after last year's fix?
I bet that thunderbirds build system isn't invoking eslint. If you
file a bug and assign me, I can look into it.
I could do that, but Philipp (who implemented eslint in calendar last
year) probably knows the status. I'd like his input first.
Philipp, is eslint working on calendar checkins, or is there some other
automated way that it is checked?
As an example, calendar/base/src/calCachedCalendar.js shows eslint
indent error on this code:
defineForwards(calCachedCalendar.prototype, "mUncachedCalendar",
["setProperty", "deleteProperty",
"isInvitation", "getInvitedAttendee", "canNotify"],
["type", "aclManager", "aclEntry"],
["id", "name", "uri", "readOnly"]);
:rkent
R Kent James via Maildev wrote on 20.07.2017 02:27:
On 7/19/2017 3:56 PM, Ben Bucksch via Maildev wrote:
For the other points , could you please give a concrete example which
existing code you would format how? Details matter here, we care down
to whitespace. This avoids that they reformat 20 classes and then
have to change them all.
Yes of course that would be part of the process, and I expect there to
be controversies there. The first step is to understand what the
issues are likely to be, and agree on the desired style going forward.
I tested today running eslint on some of the /mail files, using the
eslint rules from the Calendar project. A typical file has 99+ errors.
Most of those are just a few style differences, which we are going to
have to think about.
The main policy question is, do we want to provide linting using
eslint on other directories in comm-central, including accepting all
of the likely whitespace changes that will result from such a cleanup?
Personally, no. I do not think it's a good idea to re-format the
whitespace in all of TB code.
This will break all existing patches.
And it will make e.g. Postbox merging with TB harder.
I don't think our source code is in that bad shape.
For example, I often use spacing liberally, to express a certain complex
logic and make the code more readable. Often, such tools break that
logical formatting. I do not think that is a good idea.
When I spoke about "whitespace" above, I meant when you re-format
.prototype-based definitions to ES6 classes. If you do that, we will
want to define exactly how we want our class definitions to look like,
and will want the students (not a linter) to use that when they create
the ES6 classes. That's what I meant above.
(See how I used 2 linebreaks above to separate two different new ideas?
Most linters complain about such things, creating worse code.)
Ben
On 7/19/2017 11:35 AM, R Kent James via Maildev wrote:
- Replace older prototype-based class definitions with ES6 classes.
ES6 classes don't have a mechanism that let you do the QueryInterface:
XPCOMUtils.generateQI([]) kind of method binding. That makes it rather
annoying to interface with XPCOM in ES6 classes.
- Replace the various approaches to module management within
Thunderbird code with ES6 modules (import and export). This would
include not only Mozilla (jsm) modules, but also many <script> tags
within XUL.
The last I checked (admittedly, some time ago), es6 modules weren't
ready yet in Gecko. ES6 modules don't necessarily follow the same
semantics as Components.utils.import--in particular, with reference to
being able to have a module that contains a single cross-script global
that can be used a global database (i.e., IIRC, ES6 modules act more
like the subscript loader, not Components.utils.import).
4. DeCOMtamination of JavaScript code. Although I managed to make a
complex XPCOM strategy work in JsAccount, I came away convinced that
this had too many gotchas to recommend to others. XPCOM in pure
JavaScript code is useful as a method of clearly defining types, but
there are other ways of accomplishing the same thing that are safer
and more performant.
The ultimate challenge for deCOM is that XPIDL and XPCOM are the main
decent way forward for communicating between C++ and JS. This makes it
rather difficult to actually try deCOMing stuff, because much of our
backend is C++ and the frontend is JS, necessitating the use of XPCOM
for all coordination. And when you try doing partial merges, you then
have to throw together some interfaces to make it work (that's part of
the reason why the compose attachment xpidl is a mess).
--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist
On 7/20/17 2:46 AM, R Kent James wrote:
On 7/19/2017 5:32 PM, Tom Prince via Maildev wrote:
On Wed, Jul 19, 2017 at 6:28 PM R Kent James via Maildev
<maildev@lists.thunderbird.net
mailto:maildev@lists.thunderbird.net> wrote:
On a related issue, even the Calendar files are showing a few linting
errors. The Mozilla linting page claims eslint: "runs on
treeherder for
every check-in." Can anyone verify that? That is, is there an
automatic
test that runs that will catch linting errors once enabled? If
so, why
is Calendar still having issues after last year's fix?
I bet that thunderbirds build system isn't invoking eslint. If you
file a bug and assign me, I can look into it.
I could do that, but Philipp (who implemented eslint in calendar last
year) probably knows the status. I'd like his input first.
Philipp, is eslint working on calendar checkins, or is there some
other automated way that it is checked?
I wish eslint were working on calendar checkins, then I wouldn't have to
go on fixing it every one in a while and pestering MakeMyDay for reviews
(and then forgetting to clean up and push the patch, as I believe is
happening right now ;-).
mach eslint does not work out of the box at the moment. There are a few
path issues due to c-c being the topsrcdir where I never found an easy
solution for. It did work locally with a number of gross hacks like
symlinking the tools/ directory in m-c to c-c, but that is not something
I would recommend. In the end I found it was easier to work on bug
1366606, after which mach eslint works perfectly modulo bug 1366714. I
haven't been able to continue that lately.
Given we have a TC expert now it might be worth picking it up again and
running a separate eslint job, although I am not sure if I can get to
this soon.
Philipp
On 7/19/2017 5:58 PM, Ben Bucksch via Maildev wrote:
Personally, no. I do not think it's a good idea to re-format the
whitespace in all of TB code.
This will break all existing patches.
And it will make e.g. Postbox merging with TB harder.
I don't think our source code is in that bad shape.
For example, I often use spacing liberally, to express a certain
complex logic and make the code more readable. Often, such tools break
that logical formatting. I do not think that is a good idea.
As I understand you, you are making two points:
-
You are not enthusiastic about linting in general as a tool.
-
You are concerned about whitespace changes and how it affects
people who try to merge into the code later. The example that you gave
was Postbox.
Concerning 1), I doubt if I will be able to change your mind, as you are
an experienced developer who has thought-out opinions on what works for
you and your teams. All I can say is that eslint is being widely adopted
in other Mozilla code, has already been adopted by the Calendar project,
and I've got a few other people who have already supported this.
Could I have pro or con from others? It would be good if we could
develop a rough consensus of the value of this.
Concerning 2), although I don't consider merging by Postbox to be an
issue we would affect our decision, there are related issues associated
with excessive code changes in merges, including in particular the need
to occasionally land post-ESR patches on ESR branches. I asked Jörg
about this today, and he said that that issue is more prevalent in C++
than in JS (and this proposal is only for JS), so he could accept this
for JS. About breaking existing patches, I believe that we can alleviate
this by doing the initial landing work on a branch, then doing a single
(or a limited number) of merges so that there are minimal points in time
where patches are broken.
Again, comments from others would he helpful in getting a sense of the
developer community here. I really need to resolve this so that I know
that these contributions would be welcomed. It is a lot of work to get
linting setup.
:rkent
On 27.07.2017 00:07, R Kent James via Maildev wrote:
<snip>maybe we could do it in smaller steps - having patches that only
re-format individual files. this, imho should make sorting out
things (eg. on rebasing other patches) easier. or can we find a
merge driver that's more intelligent about whitespace-only changes?
i've got the impression that recent git can cope w/ whitespace-only
conflicts in many cases. (didn't actually test it, but observed many
cases where I suspected fails due whitespace changes, while it went
through smoothly).
another option could be fixing whitespaces (eg. we still have lots of
tabs in the code) in those lines we're touching anyways. the basic rule
would be: use the canonical formatting (assuming we already have an
definitive policy yet ;-)), but only change lines you'd touch anyways.
--mtx
R Kent James via Maildev wrote on 27.07.2017 02:07:
Concerning 1), I doubt if I will be able to change your mind, as you
are an experienced developer who has thought-out opinions on what
works for you and your teams.
Yes, I've worked in projects with and without linting. The projects that
use linting are highly painful for me, because linters make the code
stupid. I can no longer express meaning of the code as I could before.
About breaking existing patches,
This affects /all/ patches attached to /any/ bug that are not landed yet.
I believe that we can alleviate this by doing the initial landing work
on a branch, then doing a single (or a limited number) of merges so
that there are minimal points in time where patches are broken.
This does not resolve the issue. The patches will bitrot either way.
This is a large amount of patches that you trash this way.
This causes a huge amount of extra work, for no (significant) gain.
Postbox was just one extreme example (the cost for them could easily be
in the tens of thousands of dollars), but by no means the only one.
Everybody with any change to Thunderbird, anywhere, for any reason, will
be affected.
Ben
On Wed, Jul 26, 2017 at 6:08 PM R Kent James via Maildev <
maildev@lists.thunderbird.net> wrote:
Again, comments from others would he helpful in getting a sense of the
developer community here. I really need to resolve this so that I know
that these contributions would be welcomed. It is a lot of work to get
linting setup.
As Fallen as expressed a desire for linting of calendar, I am in the
process of getting that to run on try. I also plan to starting linting all
the code that is involved in the build process.
As additional data points, mozilla is in the process of linting all their
code. Eslint is enabled and running on parts of the code (and has
apparently found and helped eliminate dead code). And there are plans to
run clang-format over the entire codebase as well. If mozilla is able deal
with the churn caused by linting, with their much greater velocity, we
should be able to handle it with our much smaller velocity.
One trick that could be used is to only enforce linting on new and changed
lines, which means that any patch that doesn't apply would need to be
adjusted anyway.
-- Tom