#19 Add prettier

已關閉
alex 請求將 2 次代碼提交從 alex/prettier 合併至 alex/master
alex commented 6 年之前

I don't know if we should do this or not.

Prettier reformats our JS for us, to keep style consistent. Changes to our (alex & florrie's) current style include:

  • Max line-length of 80, except comments
  • No spaces in object literals, including destructuring (we can't handle it separately for destructuring and literals, so..)
  • Place parens around statements as expressions (eg. return (this.a = 6))

Check out the code it generated! Gonna need opinions on this - the general idea is you run npm run prettier before you commit anything, ever.

This is in lieu of ESLint, which, if we decide to add it, would check out code for less cosmetic things such as 'prefer const over let over var.' See #20.

**I don't know if we should do this or not.** [Prettier](https://prettier.io/) reformats our JS for us, to keep style consistent. Changes to our (alex & florrie's) current style include: * Max line-length of 80, except comments * No spaces in object literals, **including destructuring** (we can't handle it separately for destructuring and literals, so..) * Place parens around statements as expressions (`eg. return (this.a = 6)`) Check out the code it generated! Gonna need opinions on this - the general idea is you run `npm run prettier` before you commit anything, ever. This is in lieu of ESLint, which, if we decide to add it, would check out code for less cosmetic things such as 'prefer const over let over var.' See #20.
(quasar) nebula commented 6 年之前
協同者

I'm not sure. Some of these changes look good; some don't. For example, I don't like the changes it makes regarding line length. Which of these is more readable?

- this.emoteStore = new EmoteStore(this, config.get('discord_emote_store_server_ids'))

+ this.emoteStore = new EmoteStore(
+   this,
+   config.get('discord_emote_store_server_ids')
+ )

How about between these two?

- const url = `https://discordapp.com/oauth2/authorize?&client_id=${client.user.id}&scope=bot&permissions=${0x00000008}&response_type=code`
- log.warning(`Bot ${chalk.blue(client.user.tag)} not connected to configured Discord server(s) - add it using the following URL:\n${chalk.underline(url)}`)
- return null

+ const url = `https://discordapp.com/oauth2/authorize?&client_id=${
+   client.user.id
+ }&scope=bot&permissions=${0x00000008}&response_type=code`
+ log.warning(
+   `Bot ${chalk.blue(
+     client.user.tag
+   )} not connected to configured Discord server(s) - add it using the following URL:\n${chalk.underline(
+     url
+   )}`
+ )
+ return null

It also occasionally breaks up some plain old function calls which don't even have any object/array parameters:

- const healthState = await actionTarget.modHealth(-amount, battle.game.guild)

+ const healthState = await actionTarget.modHealth(
+   -amount,
+   battle.game.guild
+ )

...And that line was already 76 - less than 80 - characters long.

The same type of issue happens in core/config.js, regarding the typecheck functions.

Overall, while this arguably makes things more consistent as a whole, I think it lessens the readability of the code, so I'm inclined against merging this.

(I didn't even touch on the tooling aspect of this. For instance, while many modern editors will automatically reload once a file is changed, Vim will not; the safest way to deal with frequently running prettier and to avoid the annoying "FILE CHANGED ON DISK - really write?" prompt, you'd have to :qa out of the editor, then re-open and scroll to wherever you were after running prettier. A bit of a pain. And sometimes we might just altogether forget to run prettier, which could also cause trouble.)

I'm not sure. Some of these changes look good; some don't. For example, I don't like the changes it makes regarding line length. Which of these is more readable? ```js - this.emoteStore = new EmoteStore(this, config.get('discord_emote_store_server_ids')) + this.emoteStore = new EmoteStore( + this, + config.get('discord_emote_store_server_ids') + ) ``` How about between these two? ```js - const url = `https://discordapp.com/oauth2/authorize?&client_id=${client.user.id}&scope=bot&permissions=${0x00000008}&response_type=code` - log.warning(`Bot ${chalk.blue(client.user.tag)} not connected to configured Discord server(s) - add it using the following URL:\n${chalk.underline(url)}`) - return null + const url = `https://discordapp.com/oauth2/authorize?&client_id=${ + client.user.id + }&scope=bot&permissions=${0x00000008}&response_type=code` + log.warning( + `Bot ${chalk.blue( + client.user.tag + )} not connected to configured Discord server(s) - add it using the following URL:\n${chalk.underline( + url + )}` + ) + return null ``` It also occasionally breaks up some plain old function calls which don't even have any object/array parameters: ```js - const healthState = await actionTarget.modHealth(-amount, battle.game.guild) + const healthState = await actionTarget.modHealth( + -amount, + battle.game.guild + ) ``` ...And that line was already *76* - less than 80 - characters long. The same type of issue happens in `core/config.js`, regarding the `typecheck` functions. Overall, while this arguably makes things more consistent as a whole, I think it lessens the readability of the code, so I'm inclined against merging this. (I didn't even touch on the tooling aspect of this. For instance, while many modern editors will automatically reload once a file is changed, Vim will not; the safest way to deal with frequently running prettier and to avoid the annoying "FILE CHANGED ON DISK - really write?" prompt, you'd have to `:qa` out of the editor, then re-open and scroll to wherever you were after running prettier. A bit of a pain. And sometimes we might just altogether forget to run prettier, which could also cause trouble.)
alex commented 6 年之前
所有者

That string line-length thing looks to be an issue, yeah.

Overall, while this arguably makes things more consistent as a whole, I think it lessens the readability of the code, so I'm inclined against merging this.

Makes sense, and I agree. Perhaps some opinionated ESLint rules to catch style issues, rather than reformatting, would be better? Or do we want no tooling at all? This is primarily for @joshua and other contributors that may not know / be able to properly stick to our codestyle.

That string line-length thing [looks to be an issue](https://github.com/prettier/prettier/issues/5067), yeah. > Overall, while this arguably makes things more consistent as a whole, I think it lessens the readability of the code, so I'm inclined against merging this. Makes sense, and I agree. Perhaps some opinionated ESLint rules to catch style issues, rather than reformatting, would be better? Or do we want no tooling at all? This is primarily for @joshua and other contributors that may not know / be able to properly stick to our codestyle.
alex6 年之前 關閉
請重新開啟合併請求來完成合併操作。
Sign in to join this conversation.
未選擇標籤
未選擇里程碑
未指派成員
2 參與者
正在加載...
取消
保存
尚未有任何內容