Skip to content
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

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

Merged
merged 6 commits into from
Aug 20, 2024
Merged

Conversation

npaun
Copy link
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 review from harrishancock, fhanau and danlapid 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 jasnell and anonrig August 16, 2024 16:44
samples/async-context/worker.js Outdated Show resolved Hide resolved
src/cloudflare/internal/test/images/images-api-test.js Outdated Show resolved Hide resolved
src/node/_stream_duplex.js Outdated Show resolved Hide resolved
tools/cross/format.py Outdated Show resolved Hide resolved
tools/cross/format.py Outdated Show resolved Hide resolved
tools/cross/format.py Outdated Show resolved Hide resolved
tools/cross/format.py Outdated Show resolved Hide resolved
@danlapid
Copy link
Contributor

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

tools/cross/format.py Outdated Show resolved Hide resolved
@danlapid danlapid requested a review from jp4a50 August 16, 2024 18:34
@danlapid
Copy link
Contributor

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

.github/workflows/lint.yml Outdated Show resolved Hide resolved
.github/workflows/lint.yml Outdated Show resolved Hide resolved
Copy link
Member

@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.

.github/workflows/lint.yml Outdated Show resolved Hide resolved
.github/workflows/lint.yml Outdated Show resolved Hide resolved
.prettierrc.json Outdated Show resolved Hide resolved
samples/nodejs-compat-diagnosticschannel/worker.js Outdated Show resolved Hide resolved
src/node/crypto.ts Outdated Show resolved Hide resolved
@@ -9,23 +9,23 @@
import {
downloadedBinPath,
pkgAndSubpathForCurrentPlatform,
} from "./node-platform";
} from './node-platform';
Copy link
Collaborator

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
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
Collaborator

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
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

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
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
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
Member

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
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

.git-blame-ignore-revs Outdated Show resolved Hide resolved
tools/cross/format.py Outdated Show resolved Hide resolved
@npaun npaun mentioned this pull request Aug 19, 2024
.github/workflows/lint.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
tools/cross/format.py Outdated Show resolved Hide resolved
tools/cross/format.py Outdated Show resolved Hide resolved
.prettierrc.json Show resolved Hide resolved
.github/workflows/lint.yml Outdated Show resolved Hide resolved
.github/workflows/lint.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@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 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 merged commit 1fc80ca into main Aug 20, 2024
10 checks passed
@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