Skip to content

dprint codebase #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

dprint codebase #13

wants to merge 6 commits into from

Conversation

jakebailey
Copy link
Owner

@jakebailey jakebailey commented Sep 6, 2022

The first two commits run an eslint rule for trailing commas; this is just to eliminate it from the dprint diff as making our trailing commas consistent takes up a good portion of the diff. You can view just the "Run dprint" commit to see what dprint changed past that.

There are still some TODOs:

  • The top of the stack is a change so that all of our files are linted; I have already sent this as another PR and will rebase it away.
  • We have to decide when we run this
    • My initial preference was that this is enforced at CI time, like our linting.
    • However I know someone else mentioned that they had worked on a repo where a post-merge workflow auto-fixed any code changes, which sounds good too; dprint should be fast enough that if run on push, it's unlikely that anyone would be able to pull unformatted code onto their forks/branches, so format-on-save would still be viable for those who want it.
  • There are a few minor cosmetic bugs, which have been reported on: https://github.com/dprint/dprint-plugin-typescript/issues/created_by/jakebailey
    • Reasonably, all that is left is that some comments containing visual spacing are changed, but I think that those ones should actually be JSDoc.
  • The line length is set arbitrarily high; IMO we should take this opportunity to go down to something like 120. We have many files with ridiculous sizes. See also dprint codebase + lineWidth=120 and operatorPosition=nextLine #18.
  • I have operatorPosition set to maintain; I would prefer nextLine (I find this much more readable), but right now we have a mix of both, but mostly sameLine. See also dprint codebase + lineWidth=120 and operatorPosition=nextLine #18.
  • ESLint-related formatting rules need to be removed, as they no longer apply.
    • Doing so reduced lint time by about 6 seconds!
  • Our settings.json template should list dprint as the formatter, and suggest the extension.
  • Need to decide whether or not to format src/lib.
    • IMO we definitely should, especially as some lints are dropped. We already don't lint "generated" files.
    • Probably need to also format the output of the lib DOM generator.
  • Need to vet any maintain uses, to ensure that it isn't possible to write "invalid" code and have it not be caught without our eslint rules. e.g. if statements without braces that are too big.
  • Should we also format markdown? Seems like a good idea.
    • But, the style we appear to use in our docs and blog drafts appears to be the opposite of dprint, prettier, etc.
  • Once this is committed to the repo, we should mark its commit ID in https://github.com/microsoft/vscode/blob/main/.git-blame-ignore to hide it from git blames.
  • Need to improve dprint further to allow single line import { blah } from "..." instead of forcing multi-line.

Copy link

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good start.

@@ -11,9 +11,9 @@
"terminal.integrated.profiles.linux": {
"bash": {
"path": "/bin/bash",
"icon": "terminal-bash",
},
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this undo the trailing-comma eslint rule you enabled elsewhere?

Copy link
Owner Author

@jakebailey jakebailey Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, since this is a JSON file, which aren't linted. If you open this file now in our repo, VS Code actually already complains, we just have nothing that can fix it.

Comment on lines 915 to 916
if (
expr.expression.kind === SyntaxKind.PropertyAccessExpression &&

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of this additional line, although it's OK.

@sandersn
Copy link

I'd vote for a real line limit in the eventual PR; I don't like that this version produces overlong lines.

@jakebailey
Copy link
Owner Author

I don't like that this version produces overlong lines.

I think those cases can actually be "fixed" by making sure that they aren't hanging weirdly in the original code, which I can do earlier in the stack or after in a cleanup commit. But, yes, I do think that we should have a line limit.

@jakebailey jakebailey force-pushed the dprint branch 4 times, most recently from 8e9aea7 to fb868e5 Compare September 15, 2022 18:17
@jakebailey jakebailey force-pushed the dprint branch 4 times, most recently from 089d88e to 33f4d1b Compare September 26, 2022 16:11
@jakebailey jakebailey changed the title WIP: dprint codebase dprint codebase Oct 10, 2022
@jakebailey jakebailey force-pushed the dprint branch 2 times, most recently from 3afd223 to aeb6799 Compare December 7, 2022 22:38
@jakebailey jakebailey force-pushed the dprint branch 2 times, most recently from b4fbbe3 to e254ff6 Compare December 17, 2022 22:31
@jakebailey jakebailey force-pushed the dprint branch 7 times, most recently from cbf8f89 to b79f86d Compare January 22, 2023 04:57
@jakebailey jakebailey force-pushed the dprint branch 2 times, most recently from 1c103c2 to d79beb2 Compare February 10, 2023 03:11
@jakebailey jakebailey force-pushed the dprint branch 2 times, most recently from 4ffc0e3 to 1013b6a Compare March 4, 2023 23:35
@jakebailey jakebailey force-pushed the dprint branch 2 times, most recently from 3c4c7f9 to 79c5998 Compare May 19, 2023 06:15
@jakebailey
Copy link
Owner Author

microsoft#54820

@jakebailey jakebailey closed this Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants