-
Notifications
You must be signed in to change notification settings - Fork 182
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
Add --check
option to format command
#1962
Conversation
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. |
There was a problem hiding this 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?
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
cli/index_test.ts
Outdated
expect(checkResult.stderr).contains("unformatted.sqlx"); | ||
|
||
// Format the files (without check flag) | ||
await getProcessResult(execFile(nodePath, [cliEntryPointPath, "format", projectDir])); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
@Caridan |
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! |
Overview
This PR implements the Feature Request to add a
--check
flag to theformat
command.Changes
--check
flag to the format commandHow it works
When used with
--check
, the command:Example usage
Testing
I've tested both the regular format mode (which continues to work as before) and the new check mode.
In check mode:
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