Proposal to replace Clang Format with ESLint and Prettier in phosphor-webui

[Ed Tanous]

On 6/14/19 10:12 AM, Derick wrote:
> 
> I am ok if we don't want to use Prettier for JavaScript, but
> clang-format does not

Never really thought of that, and that's a fair criticism of using
clang-format for the web stuff.  I do have prettier hooked into my
editor for the scss files, so I guess I never really thought about it.

> support SCSS and we would like to have consistent formatting in SCSS as well.
> We wouldn't expect it to stop the builds, just to ensure code quality. I want
> to use the .prettierrc file to keep it consistent for anyone that uses
> Prettier and
> we can easily ignore .js files in the config. We can also make this a
> work in progress
> if we don't want to try and resolve all the files up front.
> 
>> If ESlint is really something you want to tackle, by all means, but in
>> terms of value to the end user, I suspect it's a wash.
> 
> I feel the benefit to the user is code quality and to the developer it
> is efficiency.
> You're correct, the formatting is not something that results in bugs that
> require rework. However, clang-format does not surface any potential errors.
> It is simply a formatting tool and not one that is widely used in the JavaScript
> community (https://www.npmtrends.com/clang-format-vs-eslint-vs-prettier).
> ESLint is extremely helpful to people using IDE/Editors like Visual Studio
> Code due to it's integration and showing the developer potential problems
> in their code as they are writing it. Personally, I have found the
> formatting of
> ESLint with the Google shared config preferable than clang-format and more
> consistent with the formatting most JavaScript projects use.

Maybe this is some of the disconnect.  When I went through this
exercise, the only thing it was doing was bumping some braces around to
different locations, which is already consistent across the project.  I
didn't really see a significant impact.  If you think the google config
makes you more efficient, by all means, I can get behind it, that just
wasn't my experience.

> 
>> If you start with eslint-config-google, the changes are a lot less
>> invasive, as we're pretty close to being compliant.
> 
> We want to use ESLint based on its ability to catch errors. I did
> start with Google's
> shared eslint configuration file and it is less invasive. There are
> still a few issues to
> resolve with that configuration, but if that is the tradeoff for using
> ESLint, I'm good with that.

This is kind of what I'm talking about.  I had the same hope, but I
didn't really see a lot of errors being caught that would've been a real
production issue.  With that said, it looks like you turned on a heck of
a lot more checks than I did.  Did you find any patterns that the UI
consistently got wrong?  Were there checks that identified real
functional issues in the webui (memory leaks, bad pages, race
conditions, ect)

I'm scrolling through your changeset, and most of it seems like
whitespace.  The brace rules are different.  Object keys seem like
they're always quoted.  CSS pixel alignments take the form of "0.6em"
versus ".6em".  Not a lot of these improve the readability a lot IMO.

> 
>> At one point I had looked at moving forward with Angular 2 and
>> typescript, and tried to sketch out the path to get there, but it
>> quickly got out of hand.
> 
> Agree, that will require effort and time and our team doesn't have the
> bandwidth for that.
> 
Yep.  I wasn't asking your team to do it.  I was more pointing out that
a lot of the stuff that I was really wanting out of this exercise:
(static analysis, transpiration correctness guarantees, type safety,
increased code review efficiency with automation), were only available
or useful when used from a typescript application.


All in all, if this is something you want, I can get behind it.