Skip to content

Add TS/JS/JSON formatting to the project#2543

Merged
npaun merged 6 commits intomainfrom
npaun/add-js-formatting
Aug 20, 2024
Merged

Add TS/JS/JSON formatting to the project#2543
npaun merged 6 commits intomainfrom
npaun/add-js-formatting

Conversation

@npaun
Copy link
Copy Markdown
Member

@npaun npaun commented Aug 16, 2024

This PR extends @danlapid's work in #2505 to format JS/TS/JSON code too.

@npaun npaun changed the base branch from main to dlapid/add-cpp-formatting August 16, 2024 15:40
@npaun npaun linked an issue Aug 16, 2024 that may be closed by this pull request
@npaun npaun marked this pull request as ready for review August 16, 2024 16:29
@npaun npaun requested review from a team as code owners August 16, 2024 16:29
@npaun npaun requested a review from a team August 16, 2024 16:29
@npaun npaun requested a review from a team as a code owner August 16, 2024 16:29
@npaun npaun requested review from danlapid, fhanau and harrishancock and removed request for a team August 16, 2024 16:29
@npaun npaun force-pushed the npaun/add-js-formatting branch 2 times, most recently from d347b12 to 5c3ff67 Compare August 16, 2024 16:39
@npaun npaun requested review from anonrig and jasnell August 16, 2024 16:44
@danlapid
Copy link
Copy Markdown
Collaborator

danlapid commented Aug 16, 2024

I didn't feel like doing it so who am I to throw it on you but if you feel like adding bazel take a look at https://github.com/bazelbuild/buildtools/blob/main/buildifier/README.md
basically
linting:
buildifier --lint=warn path/to/file
buildifier --lint=fix path/to/file
formatting
buildifier --mode=check path/to/file
buildifier --mode=fix path/to/file

@danlapid danlapid requested a review from jp4a50 August 16, 2024 18:34
@danlapid
Copy link
Copy Markdown
Collaborator

Do we want to use prettier for markdown as well?
I've never tried it before so this is a really open ended question

Copy link
Copy Markdown
Contributor

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

I think this is extremely hard to review and land without making sure we don't break anything. What we should do is, to enable prettier new rules on a specific folder, and divide the large chunk to smaller chunks while opening specific pull-requests according to that.

Also, I think we should use a specific prettier config that reduces the changes, and later iterate on finding the "sweet" formatting config that fits to our needs.

downloadedBinPath,
pkgAndSubpathForCurrentPlatform,
} from "./node-platform";
} from './node-platform';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These files have already been formatted with prettier, mostly—it seems like this change is just to change the ' to "?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In this discussion, several colleagues said they preferred single quotes, so I decided to apply it throughout the project.

#2543 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fair enough—as long as we're consistent. Worth noting that we use " in workers-sdk

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Obvs not a big deal but I prefer double quotes since JSON requires them, and being consistent with workers-sdk would be a nice benefit

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Quote fight!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm on team single quotes, but it's not a big deal for me either.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can use different configuration on different folders. We just need to add .prettierrc to the folder where we want to have double quotes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I would prefer to have a unified style in all of workerd, but I'm not opposed to per-folder if there is a strong reason

@npaun npaun mentioned this pull request Aug 19, 2024
@npaun npaun force-pushed the npaun/add-js-formatting branch from c59be19 to 20a4606 Compare August 20, 2024 16:21
@npaun npaun force-pushed the npaun/add-js-formatting branch from 20a4606 to 2b0b22f Compare August 20, 2024 16:33
Copy link
Copy Markdown
Contributor

@garrettgu10 garrettgu10 left a comment

Choose a reason for hiding this comment

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

I only looked at the pyodide folder but it looked nice, thanks!

@anonrig anonrig mentioned this pull request Aug 20, 2024
@npaun npaun force-pushed the npaun/add-js-formatting branch from 2b0b22f to c08dd6c Compare August 20, 2024 17:45
@npaun npaun changed the base branch from dlapid/add-cpp-formatting to main August 20, 2024 17:45
@npaun npaun force-pushed the npaun/add-js-formatting branch 3 times, most recently from cbb920b to 93d2934 Compare August 20, 2024 18:03
@npaun npaun force-pushed the npaun/add-js-formatting branch from 93d2934 to a666f32 Compare August 20, 2024 18:04
@npaun npaun force-pushed the npaun/add-js-formatting branch from a666f32 to d5b2e51 Compare August 20, 2024 18:05
@npaun npaun requested a review from jasnell August 20, 2024 18:15
@npaun npaun merged commit 1fc80ca into main Aug 20, 2024
@npaun npaun deleted the npaun/add-js-formatting branch August 20, 2024 18:37
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.

Code formatting: js+ts

7 participants