[IMP] Add Prettier and ESLint with --fix

[yajo]

This commit includes:

• Using https://prettier.io/ for supported file formats (these include CSS, SCSS, JS, HTML, MD, YAML and others; see website).
• Configure ESLint using eslint-config-prettier and removing stylistic checks to avoid conflicts with Prettier.
• Add Promise as a builtin for v13 (this was missed before, so I take this chance to add that).
• Format properly .travis.yml so that when it gets merged downstream it gets less diffs when passed trhough Prettier.
• Use eslint --fix so we get less warnings and more fixes.

Some insights:

• See [DO NOT MERGE] Prettier test web#1441 [IMP] Add Prettier and ESLint with --fix #618 (comment) as an example of how a web repo looks after this change.
• A beta XML Prettier plugin exists, which could be used in the future to autoformat XML files too!
  • But it's not usable for us right now, as you can see in Commented elements are uncommented prettier/plugin-xml#13 (comment)
  • Once that's fixed, we can use a configuration similar to How to integrate in pre-commit? prettier/plugin-xml#17 (comment) to integrate here.
• XML plugin included.

@Tecnativa @sbidoul @moylop260 OCA/maintainer-tools#323 (comment) TT20357

[moylop260]

the concept looks good to me
thanks!

[lmignon]

@yajo Nice improvement!

[sbidoul]

I tried this on my favorite repo and found a few potential issues:

• it fiddles with .md files, including rewriting LICENSE.md, is that what we want?
• it also fiddles with .html files (there is a doc template in mis builder where the prettier output is not that pretty I'd say)
• also, eslint complains

[Error - 10:01:53 PM] Error: Failed to load config "prettier" to extend from.
Referenced from: /home/sbi-local/ACSONE/OpenErp/odoo-10/src/mis-builder/.eslintrc

Otherwise, for .js, .yml, and .css files, it looks good 👍

[yajo]

• it fiddles with .md files, including rewriting LICENSE.md, is that what we want?

I think yes, autoformatting is good everywhere IMHO, MD included. We can add the exception for LICENSE files however, although it's weird that your license is written in MD because I have not seen any LICENSE provider that uses MD. They tend to use raw txt instead. Do you have an example?

• it also fiddles with .html files (there is a doc template in mis builder where the prettier output is not that pretty I'd say)

Again if you can point to the example... You can see I ignored for instance static/description where the bot generates the HTML readme; we can add more exceptions.

• also, eslint complains

You mean eslint out of pre-commit, right? For instance when using an integrated eslint on your IDE... Well, you see that it needs a new npm dependency that includes some configs. You have to make sure that the npm package eslint-config-prettier is available to the node environment where your local eslint is running. In the pre-commit environment, pre-commit will install that dependency now, though.

[sbidoul]

.md files, LICENSE.md

You can look in the mis-builder repo (I think I got the license from the GNU website).

.html

Example in OCA/mis-builder, docs directory.

So yeah, to avoid bad surprises, I would tend to restrict prettier to an explicit list of formats (.js, .css and friends, and .xml when it's stable).

eslint out of pre-commit, right? For instance when using an integrated eslint on your IDE

Yes exactly. I think there is huge value to have .eslintrc working out of the box so dummies like me do not have to ask themselves how to make their IDE work. Since we will be maintaining these configs automatically, a little bit of redundancy does not hurt.

[yajo]

You can look in the mis-builder repo (I think I got the license from the GNU website).

I see, yes it's downloadable from GNU as MD.

OK, I added an exclusion pattern to avoid touching license files. 😊

Example in OCA/mis-builder, docs directory.

Excluded now.

So yeah, to avoid bad surprises, I would tend to restrict prettier to an explicit list of formats (.js, .css and friends, and .xml when it's stable).

That's basically what I'm proposing here. Those formats are JS, Less, CSS, SCSS, HTML and MD. It's just that it's implicit, but since the prettier version is pinned, there'll be no surprises there.

I think there is huge value to have .eslintrc working out of the box so dummies like me do not have to ask themselves how to make their IDE work. Since we will be maintaining these configs automatically, a little bit of redundancy does not hurt.

I have a mixed feeling here.

On one side, you're right that it's good to have ides working out of the box, but it's also true that what really works out of the box is pre-commit, and it works even if you edit files with cat, so it's a pretty good out-of-the-box integration on any environment.

On the other side, a problem maintaining .eslintrc is that it is a JSON file, so we cannot have i.e. comments. There's no way to warn maintainers // hey, these settings come from that package, which we are not including because blahblah, so all you see is a big file with tons of settings and it's not obvious why they are there. However, now you immediately see "extends": ["prettier"], and that makes it obvious that you need some extension to make it work. I have to admit that it's also not so obvious that the package you need is eslint-config-prettier, though...

Balancing both things, I personally prefer the maintenance side of the problem. The dev side can be fixed very easily with an entry in the contributing guideline: "How to set up a development environment". Now that we have bots 🤖 we can instruct them to add a link to that section in all repos (or modules) READMEs.

[sbidoul]

That's basically what I'm proposing here. Those formats are JS, Less, CSS, SCSS, HTML and MD

I'd be more comfortable to leave out html and md.

"extends": ["prettier"], and that makes it obvious that you need some extension to make it work

Can't we include the prettier part of the config in a commented block or something? We want to keep it as simple as possible for contributors even if it's a bit less comfortable for maintainers of the config (i.e. you and me for now), and currently eslint works out of the box with the .eslintrc we provide for 13.0 branches, so requiring additional config is a regression IMO.

[yajo]

I'd be more comfortable to leave out html and md.

why do you want ugly MD/HTML files around? 🤔

I'm reluctant to exclude those. The same reasons we want code autoformatters for other formats apply for MD and HTML, and in any case, the autogenerated index.html files are excluded and the split README files are in RST so they are not affected, so it's not like that's gonna mean much of a problem for us.

If you are worried about reducing diff... then we shouldn't use code formatters at all; but I guess we already passed that milestone, so I see no problem there.

Can't we include the prettier part of the config in a commented block or something?

Nope. JSON doesn't support comments.

[yajo]

Simplifying .eslintrc is going to be easier than I thought:

• I will convert to YAML, which supports richer syntax including comments. It is supported by ESLint out of the box.
• I will remove all disabling rules. It turns out all rules are diabled by default.
• Since all rules are disabled by default, there's no need to include the eslint-config-prettier plugin because it's only disabling things that are disabled already.

I'm going to include a link to this comment, explaining that style rules shouldn't be included (those are handled by Prettier; we only need rules that check AST patterns), and that we shouldn't include any rule listed in https://github.com/prettier/eslint-config-prettier/blob/v6.6.0/index.js.

[yajo]

This should be ready to merge 😊

[sbidoul]

The .md prettying changes the addons table generated by the bot in README.md, so it will loop.

[yajo]

The .md prettying changes the addons table generated by the bot in README.md, so it will loop.

I added prettier ignore marks to README template, to avoid that.

I also opened OCA/maintainer-tools#436 to avoid different header styles.

That should fix the problem. 😊

[sbidoul]

I added prettier ignore marks to README template, to avoid that.

Does this mean all README.md need to be updated manually?

[yajo]

I was supposing it is an automatic process also... It can be done with a sed command maybe.

There's the possibility to make the script inject the prettier lines inside instead, but prettier converts [//]: # (addons) into [//]: # "addons". Not a big problem if we add that to the regexp, though. Next run of prettier would touch it and it'd be done forever.

[yajo]

So?

[moylop260]

@guewen
Your opinion is welcome here

@sbidoul
Do you have feedback?

[sbidoul]

I'm fine with the js and yml parts.

I checked the last comments again and it seems to me we are still blocked on README.md

I was supposing it is an automatic process also... It can be done with a sed command maybe.

README.md files are maintained manually.

The easiest way out is still to leave .md files out of prettier's hands, IMO.

[yajo]

Given we're going to add a big reformatter change, I took the chance to update some hooks. I tested this config and it works fine.

[yajo]

The email is ready, I just need to know the schedule if possible. Otherwise you can just reply to it in the mailing list to tell that.

[sbidoul]

I can try to prepare a bit this week-end and apply it on all repos the week-end of February 15th.

[yajo]

OK, then can this be merged already? It won't affect until you deploy the changes actually. I wonder if you want me to apply these manually to web and website repos, in PRs, given they contain most js code and will maybe produce some errors in eslint... It can also serve as playground. El vie., 24 ene. 2020 a las 11:14, Stéphane Bidoul (< [email protected]>) escribió:

…

I can try to prepare a bit this week-end and apply it on all repos the week-end of February 15th. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#618?email_source=notifications&email_token=AAHNXDN5754YGMBFYCL37Q3Q7LEPZA5CNFSM4JPB4SDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ2O7PI#issuecomment-578088893>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHNXDLE33HHCK35OKSSYYDQ7LEPZANCNFSM4JPB4SDA> .

-- Jairo Llopis

[sbidoul]

On Fri, Jan 24, 2020 at 12:23 PM Jairo Llopis ***@***.***> wrote: OK, then can this be merged already? It won't affect until you deploy the changes actually.

Sure I wonder if you want me to apply these manually to web and website repos,

in PRs, given they contain most js code and will maybe produce some errors in eslint... It can also serve as playground.

No problem, go ahead.

…

-sbi

[yajo]

I don't have merge permissions here, could anybody merge please?

[lmignon]

I'm late into this conversation but does that mean we're going to fix all the migration PRs that haven't been merged yet. It's a lot of work, even though I know it's for the best...

[yajo]

I don't think all will need to be fixed, but maybe some. As migrations usually don't collide (all is new code), most chances are that this will mean just do a rebase and an amend. El vie., 24 ene. 2020 a las 13:50, Laurent Mignon (ACSONE) (< [email protected]>) escribió:

…

I'm late into this conversation but does that mean we're going to fix all the migration PRs that haven't been merged yet. It's a lot of work, even though I know it's for the best... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#618?email_source=notifications&email_token=AAHNXDKZM7LNACW2N33SD4TQ7LW2LA5CNFSM4JPB4SDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ22YRY#issuecomment-578137159>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHNXDJATWKTCD2A5YMFXRDQ7LW2LANCNFSM4JPB4SDA> .

-- Jairo Llopis

[lmignon]

you are right @yajo Neverthless, in any case the PRs will no longer be mergeable. 😱 😏

[yajo]

Yes, I guess the amend will be required anyway.

Merge them quick before february 15th! 😁

[guewen]

@sbidoul If I propose OCA/stock-logistics-warehouse#828 in this repo during the next week, do you think it can be included in the next pre-commit files update?

[sbidoul]

@guewen Sure!