这是indexloc提供的服务,不要输入任何密码
Skip to content

Add --check option to format command #1962

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

Merged
merged 6 commits into from
Apr 26, 2025

Conversation

AobaIwaki123
Copy link
Contributor

Overview

This PR implements the Feature Request to add a --check flag to the format command.

Changes

  • Added a new --check flag to the format command
  • Modified the format command implementation to support check mode without modifying files
  • Enhanced the output formatting to clearly show which files need formatting
  • Made the command return exit code 1 if any files need formatting in check mode

How it works

When used with --check, the command:

  1. Analyzes each file that would be formatted
  2. Compares the current content with what it would be if formatted
  3. Reports files needing formatting without modifying them
  4. Returns exit code 1 if any files need formatting

Example usage

# Check if all files are formatted correctly without modifying them
dataform format --check

# Check specific files
dataform format --check --actions="definitions/my_model.sqlx" 

Testing

I've tested both the regular format mode (which continues to work as before) and the new check mode.

In check mode:

  • When all files are formatted correctly: displays success message and returns exit code 0
  • When files need formatting: displays the list of files needing formatting and returns exit code 1

This implementation follows similar patterns used in other code formatting tools like Prettier, ESLint, and Black, making Dataform better suited for modern CI/CD pipelines and development workflows.

Closes #1961

@AobaIwaki123 AobaIwaki123 requested a review from a team as a code owner April 22, 2025 13:40
@AobaIwaki123 AobaIwaki123 requested review from Tuseeq1 and removed request for a team April 22, 2025 13:40
Copy link

google-cla bot commented Apr 22, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@Ceridan Ceridan requested review from Ceridan and removed request for Tuseeq1 April 22, 2025 15:16
Copy link
Contributor

@Ceridan Ceridan left a comment

Choose a reason for hiding this comment

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

Thank you a lot for the contribution and very detailed PR description. Very appreciate that!

Since we touched this code part, may I ask you to add basic tests to index_test.ts for formatting, please?

Copy link
Contributor

@Ceridan Ceridan left a comment

Choose a reason for hiding this comment

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

The changes looks good, thank you a lot for your time and willingness to improve the change. I left two minor comments.

I ran the whole test suite, the change looks fine.

// Return error code if there are any formatting errors
const failedFormatResults = results.filter(result => !!result.err);
if (failedFormatResults.length > 0) {
return 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we please add the following for consistency:

printError(`${failedFormatResults.length} file(s) failed to format.`);

The files themselves should be already printed as we discussed in the thread.

expect(checkResult.stderr).contains("unformatted.sqlx");

// Format the files (without check flag)
await getProcessResult(execFile(nodePath, [cliEntryPointPath, "format", projectDir]));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we please add expect(checkResult.exitCode).equals(0); check after this call?

@@ -303,5 +303,53 @@ select 1 as \${dataform.projectConfig.vars.testVar2}
},
warehouseState: {}
});

// Create a correctly formatted file
Copy link
Contributor

Choose a reason for hiding this comment

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

One more thing actually. Could we extract formatting to a separate test? You can just copy paste the setup from the original test. It will be cleaner test will be more focused on formatting only.

@AobaIwaki123
Copy link
Contributor Author

@Caridan
I pushed changes you mentioned!

@Ceridan
Copy link
Contributor

Ceridan commented Apr 26, 2025

@Caridan I pushed changes you mentioned!

LGTM! Thank you a lot! I will merge your changes and plan to build a new binary on Monday.

@Ceridan Ceridan merged commit 6aff461 into dataform-co:main Apr 26, 2025
2 checks passed
@AobaIwaki123 AobaIwaki123 deleted the feat/add-format-check-option branch April 26, 2025 13:52
@AobaIwaki123
Copy link
Contributor Author

LGTM! Thank you a lot! I will merge your changes and plan to build a new binary on Monday.

Thanks! I appreciate your help and a lot of time to review!
I was really enjoyed!

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.

[Feature Proposal] Add --check flag to format command for CI/CD pipelines
2 participants