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

Fix: global_conf not taking priority #162

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Conversation

kiliantyler
Copy link
Contributor

Based on the line in the docs:

If the flag -global_conf is passed, all other steps will be circumvented and the config file will be discovered from the system config directory

This logic was backwards. Current functionality searches for the -conf path and when one isn't passed (like when using -global_conf only) it immediately returns and does not search for the global_conf. This causes the config file from the current working directory to be picked up before the global conf does even when the flag is set explicitly.

Examples A, C, D are given so that existing & future functionality can be shown to be the same. However Example B is the current and fixed issue.

Functionality comparisons:

Scenario A:

  • Config exists in local directory
  • Global config exists
  • No flags passed in

Expected result: Uses local config
Existing Result: Uses local config
New Result: Uses local config

Existing

❯ yamlfmt -debug config
[DEBUG]: No config path specified in -conf
[DEBUG]: Found config at /Users/kilian/test/current-dir/.yamlfmt
[DEBUG]: Found config at /Users/kilian/test/current-dir/.yamlfmt

New

❯ yamlfmt -debug config
[DEBUG]: No config path specified in -conf
[DEBUG]: Found config at /Users/kilian/test/current-dir/.yamlfmt
[DEBUG]: Found config at /Users/kilian/test/current-dir/.yamlfmt

Scenario B:

  • Config exists in local directory
  • Global config exists
  • -global_conf passed in

Expected Result: Uses global config
Existing Result: Uses local config
New Result: Uses global config

Existing

❯ yamlfmt -debug config -global_conf
[DEBUG]: No config path specified in -conf
[DEBUG]: Found config at /Users/kilian/test/current-dir/.yamlfmt
[DEBUG]: Found config at /Users/kilian/test/current-dir/.yamlfmt

New

❯ yamlfmt -debug config -global_conf
[DEBUG]: Using -global_conf flag
[DEBUG]: Found config at /Users/kilian/.config/yamlfmt/.yamlfmt

Scenario C:

  • Config exists in local directory
  • Global config exists
  • -conf flag passed in

Expected Result: Uses -conf specified file
Existing Result: Uses -conf specified file
New Result:

Existing

❯ yamlfmt -debug config -conf /Users/kilian/test2/yamlfmt.yaml
[DEBUG]: Using config path /Users/kilian/test2/yamlfmt.yaml from -conf flag

New

❯ yamlfmt -debug config -conf /Users/kilian/test2/yamlfmt.yaml
[DEBUG]: Using config path /Users/kilian/test2/yamlfmt.yaml from -conf flag

Scenario D:

  • Config exists in local directory
  • Global config exists
  • -global_conf & -conf are passed in

Expected Result: -global_conf takes precidence
Current Result: -global_conf takes precidence
New Result: -global_conf takes precidence

Existing

❯ yamlfmt -debug config -global_conf -conf /Users/kilian/test2/yamlfmt.yaml
[DEBUG]: Using -global_conf flag
[DEBUG]: Found config at /Users/kilian/.config/yamlfmt/.yamlfmt

New

❯ yamlfmt -debug config -global_conf -conf /Users/kilian/test2/yamlfmt.yaml
[DEBUG]: Using -global_conf flag
[DEBUG]: Found config at /Users/kilian/.config/yamlfmt/.yamlfmt

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 for catching this, and I love the detailed PR description with debug logs! This does seem to fix my mistake.

I have made some suggestions for the comments, since with the fix the comments describing the behaviour no longer line up.

cmd/yamlfmt/config.go Outdated Show resolved Hide resolved
cmd/yamlfmt/config.go Outdated Show resolved Hide resolved
cmd/yamlfmt/config.go Outdated Show resolved Hide resolved
@kiliantyler
Copy link
Contributor Author

Ahh what a silly mistake not moving the comments!

Should be all set

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.

Looks good to me, thank you for the PR!

@braydonk braydonk merged commit 4ae531d into google:main Feb 29, 2024
5 checks passed
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