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

chore: auto format some TypeScript files #286

Merged
merged 1 commit into from
Aug 29, 2024
Merged

Conversation

lym953
Copy link
Contributor

@lym953 lym953 commented Aug 28, 2024

What does this PR do?

Auto format some TypeScript files

Motivation

The next PR (#285) will touch lots of files, and saving those files on VS Code on my laptop will auto-format those files, mixing formatting changes and other changes in the same PR, so I want to separate out the formatting changes to this PR to make changes easier to review.

I can also disable "format on save" on my IDE, but I think it's a good thing to keep code in good format.

Testing Guidelines

npx projen build, no error

Additional Notes

Types of Changes

  • Bug fix
  • New feature
  • Breaking change
  • Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

  • This PR's description is comprehensive
  • This PR contains breaking changes that are documented in the description
  • This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
  • This PR impacts documentation, and it has been updated (or a ticket has been logged)
  • This PR's changes are covered by the automated tests
  • This PR collects user input/sensitive content into Datadog

@lym953 lym953 requested a review from a team as a code owner August 28, 2024 21:10
console.log(
"Instrumenting Lambda Functions in TypeScript stack with Datadog"
);
console.log("Instrumenting Lambda Functions in TypeScript stack with Datadog");

Choose a reason for hiding this comment

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

🟠 Code Quality Violation

Unexpected console statement. (...read more)

Debugging with console is not considered a bad practice, but it's easy to forget about console statements and leave them in production code. There is no need to pollute production builds with debugging statements.

View in Datadog  Leave us feedback  Documentation

@duncanista
Copy link
Contributor

Isn't it possible to format them with a plugin? Instead of relying on a formatter that is only installed in your computer?

@lym953
Copy link
Contributor Author

lym953 commented Aug 28, 2024

@duncanista Right now the format check only covers src/. Let me make it cover .ts files in integration_tests/ and examples/ as well. If format is wrong, the check will remind the developer to run prettier but won't run prettier itself.

See the format check failure on the demo commit: https://github.com/DataDog/datadog-cdk-constructs/actions/runs/10605642445/job/29394854056

@lym953 lym953 force-pushed the yiming.luo/auto-format branch 2 times, most recently from 2cb24c9 to 983592b Compare August 28, 2024 23:10
@@ -67,7 +67,7 @@ const project = new awscdk.AwsCdkConstructLibrary({
"CONTRIBUTING.md",
],
scripts: {
"check-formatting": "prettier --check src/**",
"check-formatting": "prettier --check src/** integration_tests/**/*.ts examples/**/*.ts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to find a more succinct way to get all .ts files in the repo, but couldn't find one. For example, I tried **/*.ts and */**/*.ts, but both fail to find some files in integration_tests or in examples.

@duncanista
Copy link
Contributor

@lym953 ok, thanks for the detailed explanation and the comment on what work and what didn't, approving, feel free to take the right decision!

@lym953
Copy link
Contributor Author

lym953 commented Aug 29, 2024

/merge

@dd-devflow
Copy link

dd-devflow bot commented Aug 29, 2024

🚂 MergeQueue: pull request added to the queue

The median merge time in main is 4m.

Use /merge -c to cancel this operation!

@dd-mergequeue dd-mergequeue bot merged commit 816bb26 into main Aug 29, 2024
13 checks passed
@dd-mergequeue dd-mergequeue bot deleted the yiming.luo/auto-format branch August 29, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants