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

feat: add flag to print resolved config #179

Merged
merged 5 commits into from
May 13, 2024

Conversation

kachick
Copy link
Contributor

@kachick kachick commented May 10, 2024

Suggestion

Make it possible yamlfmt could provide an overview of the final config it interprets from the config files and CLI flags.

Background

I have recently used several different versions of yamlfmt for projects with different configurations. When the behavior was different from what I expected, I was confused about whether it was due to the version of yamlfmt or a mistake in my own settings. I think it's also because the yamlfmt doesn't throw an error even when given formatter options it doesn't accept, and the number of available config file name patterns has increased from recent versions.

To my knowledge, such a feature is not uncommon in configurable linters and formatters.

  • dprint: dprint output-resolved-config
  • typescript: tsc --showConfig
  • eslint: eslint --print-config
  • rubocop: rubocop --show-cops

@braydonk
Copy link
Collaborator

braydonk commented May 11, 2024

Thank you very much for opening a PR! This is an awesome feature idea, it'll be a big help.

I apologize in advance that I'm going to suggest a complete change to the approach. This is mostly due to reasoning for structuring the code certain ways that you couldn't have known going in because I don't document it. I don't get a lot of feature PRs, so I appreciate your work! 😄


There are a couple reasons I'd prefer not to do it this way:

  • I would prefer formatters to own their config type string rather than it being in the root yamlfmt package. (I don't know if anyone is using this API boundary for individual formatters, but I have plans for it coming up)
  • I'm not a big fan of the structs needing yaml tags as well as mapstructure
  • This approach will still hide the default settings that the formatter itself will end up setting when created by the Factory
  • The term resolved is kind of overloaded, in other parts of the codebase I use the terminology resolved config to mean which config file location was discovered and used

Here's my alternative approach:

Add a new method to the yamlfmt.Formatter interface: ConfigMap() map[string]any

Implement the ConfigMap method for the basic formatter. Something like this:

func (f *BasicFormatter) ConfigMap() (map[string]any, error) {
	configMap := map[string]any{}
	err := mapstructure.Decode(f.Config, &configMap)
	if err != nil {
		return nil, err
	}
	configMap["type"] = BasicFormatterType
	return configMap, err
}

This way the formatter can maintain ownership of that BasicFormatterType value.

Upon printing configuration, use the same mapstructure.Decode API to get a map of the CommandConfig. Delete the formatter key from the resulting map.

On each of these maps, you can use yaml.Marshal directly. I tested this out and it results in similar output as what you have in the PR currently, and it means we can stick to only having mapstructure tags.

The only downside of going to a map and then yaml is that the order gets a bit whacky. For this reason, I think the nicest output result would be:

commandConfig := map[string]any{}
err = mapstructure.Decode(c.Config, &commandConfig)
if err != nil {
	return err
}
delete(commandConfig, "formatter")
out, err := yaml.Marshal(commandConfig)
if err != nil {
	return err
}
fmt.Print(string(out))

formatterConfigMap, err := formatter.ConfigMap()
if err != nil {
	return err
}
out, err = yaml.Marshal(map[string]any{
"formatter": formatterConfigMap,
})
if err != nil {
	return err
}
fmt.Print(string(out))

This results in this kind of output:

continue_on_error: false
doublestar: false
exclude: []
extensions:
    - yaml
    - yml
gitignore_excludes: false
gitignore_path: .gitignore
include: []
line_ending: lf
output_format: default
regex_exclude: []
formatter:
    disallow_anchors: false
    drop_merge_tag: false
    include_document_start: false
    indent: 2
    indentless_arrays: false
    line_ending: lf
    max_line_length: 0
    pad_line_comments: 1
    retain_line_breaks: false
    retain_line_breaks_single: false
    scan_folded_as_literal: false
    type: basic

Finally, I would recommend changing -resolved_conf to -print_conf to avoid the confusion with the usage of resolved elsewhere in the codebase (mainly in config debug logging).

For integration tests, I like the integration test you've added. I'd recommend adding another one that uses a .yamlfmt config file to set the values (I don't think I have any other tests that do this, but I'm 99% sure it works, if it doesn't then you can skip this).
BTW I don't have it documented, but there is a make target to add an integration test case that automatically makes the folder structure for you: TEST_NAME=something make command_test_case

If this is too much of a change, I would understand if you decide to close the PR and I can implement this for v0.13.0 instead. Up to you. Thanks again for the PR!

kachick added a commit to kachick/yamlfmt that referenced this pull request May 12, 2024
google#179 (comment)

* Prefer basic formatter having mapstructure rather than yaml tag in command

* Rename resolved with print

* Add integration tests for loading config files
google#179 (comment)

* Prefer basic formatter having mapstructure rather than yaml tag in command

* Rename resolved with print

* Add integration tests for loading config files
@kachick kachick force-pushed the feat-mode-output-resolved-config branch from c228b70 to 2d4315e Compare May 12, 2024 19:04
@kachick kachick marked this pull request as draft May 12, 2024 19:05
@kachick kachick marked this pull request as ready for review May 12, 2024 19:24
@kachick
Copy link
Contributor Author

kachick commented May 12, 2024

Thank you for the polite review and suggestions. 🙏

Especially, this is my first experience using mapstructure. In my understanding, it can be used for YAML marshaling here because it assumes that YAML should have the same name as the map key.

@braydonk
Copy link
Collaborator

In my understanding, it can be used for YAML marshaling here because it assumes that YAML should have the same name as the map key.

Yes that's correct. An assumption I've never actually documented, but throughout the project I essentially always use mapstructure tags in types that come from a yaml file, so if the name differs in yaml from those tag names then it won't work anyway. The reason I did this early on was because I couldn't fully type the yaml as it starts out not understanding which formatter settings it will needs to deserialize at unmarshal time. (Of course today there is only the basic formatter to this day, but I wanted to make it work for more).

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.

Thank you very much for applying the suggestions! Looks great, just one minor comment and this will be good to merge.

@@ -1,5 +1,6 @@
# Sometimes use these for testing
.yamlfmt
!integrationtest/**/.yamlfmt
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, I didn't think of that

@@ -0,0 +1,2 @@
a:
Copy link
Collaborator

@braydonk braydonk May 13, 2024

Choose a reason for hiding this comment

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

These yaml files can be removed for these -print_conf tests. They should be able to operate without them.

@kachick kachick marked this pull request as draft May 13, 2024 12:16
@braydonk
Copy link
Collaborator

braydonk commented May 13, 2024

Ah okay, forgot about that git quirk. Could you fill any folders with no files with a file called .empty? .gitkeep is good too, thanks

@kachick kachick marked this pull request as ready for review May 13, 2024 12:28
@kachick
Copy link
Contributor Author

kachick commented May 13, 2024

Thank you for quick reviews, I have updated!

@braydonk braydonk merged commit e5bdc90 into google:main May 13, 2024
7 checks passed
@kachick
Copy link
Contributor Author

kachick commented May 13, 2024

🙏 Thank you!

@kachick kachick deleted the feat-mode-output-resolved-config branch May 13, 2024 12:28
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.

2 participants