Skip to content

Conversation

okuuva
Copy link

@okuuva okuuva commented Sep 25, 2025

Fixes #61.

Copy link

google-cla bot commented Sep 25, 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.

Copy link
Collaborator

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, looks like a good start. Can you also add an integration test case that leverages a .editorconfig? The guidance for how to add a test case is here:
https://github.com/google/yamlfmt/blob/main/docs/integration-test.md#add-new-test-case

gopkg.in/ini.v1 v1.67.0 // indirect
)

require (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this need a go mod tidy; not sure why it added this extra require block.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, that's a good question. Another good one is why go mod tidy only touches go.sum...

@okuuva
Copy link
Author

okuuva commented Sep 25, 2025

Thanks for the PR, looks like a good start. Can you also add an integration test case that leverages a .editorconfig? The guidance for how to add a test case is here: main/docs/integration-test.md#add-new-test-case

Sure! I'm just thinking that this probably isn't as straight forward as I thought. The "Automatic search for .editorconfig files" section of the readme implies that the config file should be checked for every formatted file separately, in case src/foo and src/foo/bar both have a separate .editorconfig file. In which case files under src/foo/bar should potentially be formatted differently than files under src/foo...

@okuuva
Copy link
Author

okuuva commented Sep 26, 2025

Thanks for the PR, looks like a good start. Can you also add an integration test case that leverages a .editorconfig? The guidance for how to add a test case is here: main/docs/integration-test.md#add-new-test-case

Sure! I'm just thinking that this probably isn't as straight forward as I thought. The "Automatic search for .editorconfig files" section of the readme implies that the config file should be checked for every formatted file separately, in case src/foo and src/foo/bar both have a separate .editorconfig file. In which case files under src/foo/bar should potentially be formatted differently than files under src/foo...

Yup, that's the case. So basically .editorconfig support would require a feature that enables using a separate config per processed file. Looking at the code I can't see any clean way of implementing that. @braydonk is that something you'd be willing to include and support and if so, do you have any pointers on how exactly to do that? To be clear: I'm willing to do the work, just wanted to check with you first before starting such a big change.

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.

feat: use .editorconfig values as fallback config source
2 participants