#19 Add prettier

Closed
alex wants to merge 2 commits from alex/prettier into alex/master
alex commented 5 years ago

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 5 years ago
Collaborator

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 5 years ago
Owner

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.
Please reopen this pull request to perform merge operation.
Sign in to join this conversation.
No Label
No Milestone
No assignee
2 Participants
Loading...
Cancel
Save
There is no content yet.