<div dir="ltr">On Thu, Nov 26, 2015 at 2:53 PM, Mike Conley <span dir="ltr"><<a href="mailto:mconley@mozilla.com" target="_blank">mconley@mozilla.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span class="">> That's a good start. How difficult is a hg hook that runs lint on<br>> changed and added files? Or something in mozreview?<br><br></span>FWIW, I just saw bug 1196263 land in the version-control-tools repo, which might be applicable here.</div></blockquote><div><br></div><div>Yes! Good find Mike. The Engineering Productivity and relman teams plan to integrate linters, static analysis, and code coverage into MozReview. The MozReview team is currently focused on improving primary use cases to make the tool more usable. We are looking at things in parallel including:<br><br></div><div>- Sylvestre is hiring a contractor (person already identified) to work through the backlog of issues already identified by static analysis and to help identify those rules that have the best value (fixing real issues with a low false positive rate)<br></div><div>- nbp (js team) is working on JS code coverage<br></div><div>- cmanchester has been looking into C++ code coverage and how to integrate into MozReview<br><br></div><div>Ideally I think we provide feedback and rule enforcement in MozReview as it's best to identify issues when the code is being developed/reviewed. We may want to have Hg hooks as well but we need to be careful not to make Hg commits really slow. This may matter less once autoland is complete (autoland to inbound via MozReview should be done this quarter) but is still a change that we want to make with our eyes open.<br><br></div><div>In terms of static analysis/lint roll-out, we are also looking at the approach of enabling a small set of rules to begin with and increasing the rule set over time.<br></div><div><br></div><div>Lawrence<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="h5"><br><div class="gmail_extra"><br><div class="gmail_quote">On 26 November 2015 at 14:51, Dave Townsend <span dir="ltr"><<a href="mailto:dtownsend@mozilla.com" target="_blank">dtownsend@mozilla.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Thanks for starting this thread Gijs. As it happens the TAG were<br>
discussing needing to get things like this moving in our meeting<br>
yesterday.<br>
<span><br>
On Thu, Nov 26, 2015 at 5:56 AM, Mark Banner <<a href="mailto:mbanner@mozilla.com" target="_blank">mbanner@mozilla.com</a>> wrote:<br>
> On 26/11/2015 11:48, Gijs Kruitbosch wrote:<br>
><br>
> What is the quickest way we can make this bar higher today? I know devtools<br>
> and loop already have some eslint support, but it seems that ./mach eslint<br>
> can't deal with preprocessed files or with |const| . That won't work for the<br>
> vast majority of our files. How hard is it to fix that and start using this<br>
> more systematically? Patrick/Nick/Dan, can you clarify?<br>
><br>
> I suspect that preprocessing would require some sort of plugin, I suspect<br>
> writing one could be hard due to knowing whether or not to follow certain<br>
> options. However, I'd also suggest we should speed up the effort to remove<br>
> preprocessor flags from js code (bug 1150859).<br>
<br>
</span>I think this is the right call. Removing preprocessing gives us a lot<br>
of wins regardless of eslint usage so we should be doing that in<br>
preference to fixing eslint. We'll probably still have some bits left<br>
over but hopefully at that point it won't be worth the effort.<br>
<div><div><br>
> It doesn't seem like the non-existing variables thing as-is is caught by<br>
> ./mach eslint today - not even for cases where there is an argument that is<br>
> within a short Levenshtein distance of an identifier used in the function.<br>
> Does ESLint support some way of indicating available globals and enforcing<br>
> code doesn't depend on globals that aren't available? (I know that some of<br>
> the JS minification tools support things like this, so I'm a little<br>
> surprised not to see it mentioned for ESLint...)<br>
><br>
> There's no real "global" configuration for eslint today. So it will only<br>
> catch a couple of very basic things unless you add to the configuration.<br>
> You'd either need to turn on the "recommended" set, or the rule itself:<br>
> <a href="http://eslint.org/docs/rules/no-unused-vars" rel="noreferrer" target="_blank">http://eslint.org/docs/rules/no-unused-vars</a><br>
><br>
> For the globals, these can be specified in either the .eslintrc file, or<br>
> there are plugins that the devtools team have written to deal with our XPCOM<br>
> imports and other things.<br>
><br>
> One thing that might be possible short term is to set up an infrastructure<br>
> job that runs eslint on all the "shipped" JS source files (probably not as<br>
> part of mochitest because I expect it'll be too slow). That avoids the<br>
> preprocessing issue, but we'll still need it to deal with the "we use ES6"<br>
> thing...<br>
><br>
> eslint can already cope with a lot of es6. There are some constructs that we<br>
> use that are Mozilla specific, or there may be some things that are so new<br>
> they don't work in eslint yet. We can either not use them, or write plugins,<br>
> or get eslint extended.<br>
><br>
> From the experience of Hello, I would say the best way to start this would<br>
> be to have everything off (which is pretty much the default these days), but<br>
> turn on a couple of rules for an area. Then, once the infra is in place, we<br>
> can add more as we go.<br>
<br>
</div></div>How difficult is this to do. Can we get it done next week say and then<br>
maybe spend some time in Orlando tinkering with rules?<br>
<span><br>
> For Hello, we turned on a lot of rules very quickly (we had some<br>
> contributors step up for good first bugs which helped greatly!), and these<br>
> days there are possibly more we'd enable, but we're mainly working along the<br>
> lines of, "if I mentioned a nit in a review, is that something eslint could<br>
> have caught for us".<br>
><br>
> At the moment, we have a nag monitor in place, that tells us within a few<br>
> hours of landing if something has caused a regression. Getting infra of some<br>
> form would good for all of Mozilla.<br>
<br>
</span>That's a good start. How difficult is a hg hook that runs lint on<br>
changed and added files? Or something in mozreview?<br>
<span><br>
> The biggest win I find, is the editor integration. I know it can be<br>
> integrated into Atom and Sublime (probably more) to give almost real-time<br>
> updates, this means I'm fixing typos and issues before I even save/run the<br>
> code saving lots of time.<br>
</span><div><div>_______________________________________________<br>
firefox-dev mailing list<br>
<a href="mailto:firefox-dev@mozilla.org" target="_blank">firefox-dev@mozilla.org</a><br>
<a href="https://mail.mozilla.org/listinfo/firefox-dev" rel="noreferrer" target="_blank">https://mail.mozilla.org/listinfo/firefox-dev</a><br>
</div></div></blockquote></div><br></div></div></div></div>
<br>_______________________________________________<br>
firefox-dev mailing list<br>
<a href="mailto:firefox-dev@mozilla.org">firefox-dev@mozilla.org</a><br>
<a href="https://mail.mozilla.org/listinfo/firefox-dev" rel="noreferrer" target="_blank">https://mail.mozilla.org/listinfo/firefox-dev</a><br>
<br></blockquote></div><br></div></div>