Format Code with Prettier#47
Conversation
``` npm run format ```
kevinastone
force-pushed
the
prettier
branch
from
a493fa1 to
d34b3b3
Compare
|
I'm going to strongly disagree with this one. I have specific reasons for some of the stylistic choices, and prettifiers tend not to follow those guidelines. |
|
Actually, looking over it more closely, it's mostly good changes, but I have quibbles that I hope can be addressed by config changes to the prettier. I'll add comments to the commit directly. |
| public addToDB( | ||
| db_frontend: pg_db.PG | ||
| ,success_callback: () => void | ||
| ): void | ||
| { | ||
| public addToDB(db_frontend: pg_db.PG, success_callback: () => void): void { |
There was a problem hiding this comment.
Breaking out parameters line by line, as in the original, is preferred. Commas can go to the end of the line.
There was a problem hiding this comment.
Prettier collapses to a single-line when it can. That's the basic formatting algorithm: collapse-all and then progressively expand to fit under the line-length. There's not much control if you prefer always expanded to multi-line for function arguments such as this case. 😐
| this.addArea( db, area_callback ); | ||
| this.addArea(db, area_callback); |
There was a problem hiding this comment.
It shouldn't collapse whitespace like this. Whitespace is good.
There was a problem hiding this comment.
I didn't see an option for this in prettier.
| 'https://www.googleapis.com/auth/admin.directory.group.member' | ||
| ,'https://www.googleapis.com/auth/admin.directory.group' | ||
| ,'https://mail.google.com/' | ||
| ,'https://www.googleapis.com/auth/gmail.modify' | ||
| ,'https://www.googleapis.com/auth/gmail.compose' | ||
| ,'https://www.googleapis.com/auth/gmail.send' | ||
| 'https://www.googleapis.com/auth/admin.directory.group.member', | ||
| 'https://www.googleapis.com/auth/admin.directory.group', | ||
| 'https://mail.google.com/', | ||
| 'https://www.googleapis.com/auth/gmail.modify', | ||
| 'https://www.googleapis.com/auth/gmail.compose', | ||
| 'https://www.googleapis.com/auth/gmail.send', |
There was a problem hiding this comment.
This change is fine, but FYI, my original reason for leading commas like this is because adding items to the end of a list is more common than the beginning. In languages that don't allow a final comma at the end, this leading comma style helps prevent syntax errors when adding new elements. I do think the style is ugly, but it helps prevent problems.
Then I found out JavaScript added final commas to everything between ES5 and ES2017, so this ugly style isn't necessary.
There was a problem hiding this comment.
Yeah, trailing commas alleviates the benefits of some styles that focus on diff-cleanliness.
| client_id, client_secret, redirect_uris[0] ); | ||
| client_id, | ||
| client_secret, | ||
| redirect_uris[0], | ||
| ); |
There was a problem hiding this comment.
This sort of thing is good. I vastly prefer breaking out parameters line by line this way. Vertical space doesn't need to be conserved.
| 'INSERT INTO members (', | ||
| 'full_name', | ||
| ') VALUES ($1) RETURNING id', |
There was a problem hiding this comment.
This might be tough for this prettifier to handle. The 'full_name' parameter here was given an extra layer of indent, because it makes sense for the SQL. The prettifier isn't likely to grok embedded SQL like that, though. I'm guessing there's some way to say "PRETTIFIER LEAVE THIS BLOCK ALONE", but that would be tedious to add to all the embedded SQL in here.
I don't like it, but can live with it.
There was a problem hiding this comment.
Yeah, you can use inline comments to ignore specific lines (or "nodes"): https://prettier.io/docs/en/ignore.html#javascript
| export interface Logger | ||
| { | ||
| export interface Logger { |
There was a problem hiding this comment.
My personal style is that curly brackets is K&R Style. Curlies should go on their own line for important structural things (interfaces, classes, non-anonymous functions), and on the same line for internal things (conditionals, loops, etc.)
There was a problem hiding this comment.
Didn't see any option for alternative bracketing styles unfortunately.
| this.addTool( db, area_id, member_id, | ||
| (tool2_id, tool2_name) => tool2_callback( | ||
| tool2_id, tool2_name, area_id, member_id ) | ||
| this.addTool(db, area_id, member_id, (tool2_id, tool2_name) => | ||
| tool2_callback(tool2_id, tool2_name, area_id, member_id), |
There was a problem hiding this comment.
Here, I don't like the original or the new version. Parameters should break out line by line.
|
I'm less concerned about adopting any given style myself, but I do think it's a significant obstacle for newcomers and others looking to learn or contribute. So, if you're looking to grow contributors, I'd consider adopting prettier. For a beginner, being able to run https://prettier.io/docs/en/why-prettier.html covers the general philosophy and the general adoption and trend of opinionated automatic code formatters across the industry. |
2 participants
Prettier automatically formats your code so you can focus on coding rather than style and format.