Add editorconfig validation #896
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new command-line interface (CLI) for the EditorConfig Checker tool within the Atmos CLI. It includes a command for validating files against the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/editor_config.go (2)
37-57: Initialization flow is well-structured.
MergingcliConfigintocurrentConfigis clear. Consider logging the final config path for troubleshooting.🧰 Tools
🪛 golangci-lint (1.62.2)
46-46: printf: non-constant format string in call to github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Error
(govet)
113-137: Persistent flags are well-defined.
The scope of each flag is clear. If more flags appear, ensure new ones align with this pattern for consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
cmd/editor_config.go(1 hunks)go.mod(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/editor_config.go
46-46: printf: non-constant format string in call to github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Error
(govet)
62-62: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Debug
(govet)
77-77: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Error
(govet)
83-83: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
93-93: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Error
(govet)
103-103: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
🔇 Additional comments (5)
cmd/editor_config.go (3)
25-35: Command registration approach looks straightforward.
Nicely integrates with Cobra for CLI usage.
59-98: Validation logic is robust.
Checks file existence, handles dry-run, and reports errors smoothly. Good job!
🧰 Tools
🪛 golangci-lint (1.62.2)
62-62: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Debug
(govet)
77-77: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Error
(govet)
83-83: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
93-93: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Error
(govet)
100-111: Early termination flags are polite and clear.
Helps users see version or usage right away. Nicely done.
🧰 Tools
🪛 golangci-lint (1.62.2)
103-103: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
go.mod (2)
20-20: Dependency pinned to v3.0.3.
Looks good. Confirm that it aligns with your tested version.
99-99: New indirect dependencies recognized.
These libraries are typical for EditorConfig checks. Please confirm their licenses are suitable for your project.
Also applies to: 127-127, 132-132
📝 WalkthroughWalkthroughThis pull request introduces a new CLI command Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/editor_config.go (1)
26-28: Offer usage examples for the new command.Adding examples in the long description or using
cmd.Examplehelps users understand usage quickly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
cmd/editor_config.go(1 hunks)go.mod(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/editor_config.go
46-46: printf: non-constant format string in call to github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Error
(govet)
62-62: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Debug
(govet)
77-77: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Error
(govet)
83-83: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
93-93: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Error
(govet)
103-103: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
🔇 Additional comments (1)
go.mod (1)
20-20: Dependencies look good.
The newly added references appear consistent with the code. No issues detected.
Also applies to: 99-99, 127-127, 132-132
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/editor_config.go (1)
39-60:⚠️ Potential issueImprove error handling and logger usage.
The function needs improvements in error handling and logger usage:
- Use proper format strings in logger calls
- Consider returning errors instead of calling os.Exit
Apply these changes to fix the logger format strings and improve error handling:
func initializeConfig() error { u.LogInfo(atmosConfig, fmt.Sprintf("EditorConfig Checker CLI Version: %s", editorConfigVersion)) if configFilePath == "" { configFilePath = defaultConfigFilePath } var err error currentConfig, err = config.NewConfig(configFilePath) if err != nil { - logger.Error(err.Error()) - os.Exit(1) + logger.Errorf("%v", err) + return fmt.Errorf("failed to create config: %w", err) } if err := currentConfig.Parse(); err != nil { + return fmt.Errorf("failed to parse config: %w", err) } if tmpExclude != "" { currentConfig.Exclude = append(currentConfig.Exclude, tmpExclude) } currentConfig.Merge(cliConfig) + return nil }🧰 Tools
🪛 golangci-lint (1.62.2)
49-49: printf: non-constant format string in call to github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Error
(govet)
🧹 Nitpick comments (1)
cmd/editor_config.go (1)
18-25: Consider encapsulating configuration variables.The global variables could be encapsulated within a configuration struct to improve maintainability and testability.
+type editorConfigState struct { + version string + defaultConfigPath string + currentConfig *config.Config + cliConfig config.Config + configFilePath string + tmpExclude string +} +var state = &editorConfigState{ + version: "v3.0.3", // Update to match dependency version + defaultConfigPath: ".ecrc", +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/editor_config.go(1 hunks)examples/demo-stacks/.ecrc(1 hunks)tests/test_cases.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/demo-stacks/.ecrc
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/editor_config.go
49-49: printf: non-constant format string in call to github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Error
(govet)
80-80: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Error
(govet)
86-86: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
96-96: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Error
(govet)
107-107: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
🔇 Additional comments (3)
tests/test_cases.yaml (1)
139-153: Ensure version string consistency across test and implementation.
The test expects EditorConfig Checker CLI Version "v3.0.0", but the dependency in go.mod uses v3.0.3. This mismatch could cause test failures.
cmd/editor_config.go (2)
27-37: LGTM! Well-structured command implementation.
The command follows Cobra best practices with clear separation between initialization and execution.
117-134: LGTM! Well-organized flag configuration.
The flags are comprehensive and well-documented, following Cobra best practices.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/editor_config.go (1)
62-101: Consider refactoring for better testabilityThe function has multiple
os.Exitcalls which makes unit testing difficult. Consider:
- Returning errors instead of calling
os.Exit- Moving the version check to a separate function
- Centralizing error handling
Here's a suggested refactor pattern:
- func runMainLogic() { + func runMainLogic() error { config := *currentConfig - if utils.FileExists(config.Path) && config.Version != "" && config.Version != editorConfigVersion { - u.LogError(atmosConfig, fmt.Errorf("Version from config file is not the same as the version of the binary")) - u.LogError(atmosConfig, fmt.Errorf("Binary: %s, Config: %s", editorConfigVersion, config.Version)) - os.Exit(1) + if err := checkVersion(config); err != nil { + return fmt.Errorf("version check failed: %w", err) } filePaths, err := files.GetFiles(config) if err != nil { - u.LogError(atmosConfig, err) - os.Exit(1) + return fmt.Errorf("failed to get files: %w", err) } // ... rest of the logic with similar error handling + return nil } + func checkVersion(config config.Config) error { + if !utils.FileExists(config.Path) || config.Version == "" { + return nil + } + if config.Version != editorConfigVersion { + return fmt.Errorf("version mismatch: binary=%s, config=%s", + editorConfigVersion, config.Version) + } + return nil + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/editor_config.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/editor_config.go
106-106: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
🔇 Additional comments (3)
cmd/editor_config.go (3)
18-18: Align editorConfigVersion with library version
The hardcoded version v3.0.0 doesn't match the library version v3.0.3 from go.mod. This mismatch could cause confusion during version checks.
- editorConfigVersion = "v3.0.0"
+ editorConfigVersion = "v3.0.3"26-36: Well-structured command implementation!
Clean separation of concerns between initialization and execution using Cobra's PreRun and Run hooks.
117-133: Excellent flag configuration!
The flags are well-documented, comprehensive, and follow a consistent pattern. The granular disable options provide good flexibility for users.
samtholiya
force-pushed
the
feature/dev-2836-implement-atmos-validate-editorconfig
branch
from
879fb8b to
3db160d
Compare
osterman
changed the title
osterman
changed the title
… of https://github.com/cloudposse/atmos into feature/dev-2836-implement-atmos-validate-editorconfig
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/editor_config.go (1)
120-120: Fix non-constant format string warning.Address the govet warning about non-constant format string:
- config.Logger.Output(editorConfigVersion) + config.Logger.Output("%s", editorConfigVersion)🧰 Tools
🪛 golangci-lint (1.62.2)
120-120: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/editor_config.go(1 hunks)tests/test_cases.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_cases.yaml
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/editor_config.go
120-120: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
🔇 Additional comments (3)
cmd/editor_config.go (3)
20-20: Use the conventional .editorconfig filename.
As suggested in previous feedback, let's use the standard .editorconfig filename instead of .ecrc.
- defaultConfigFilePath = ".ecrc"
+ defaultConfigFilePath = ".editorconfig"28-30: Update command name and descriptions for clarity.
As suggested in previous feedback:
- Use: "editorconfig-checker",
- Short: "EditorConfig Checker CLI",
- Long: "A command-line tool to check your files against the EditorConfig rules",
+ Use: "editorconfig",
+ Short: "Validate all files against the EditorConfig",
+ Long: "Validate all files against the project's EditorConfig rules",40-63: LGTM! Well-structured configuration initialization.
The configuration initialization is well-implemented with proper error handling and logging using Atmos utilities.
… of https://github.com/cloudposse/atmos into feature/dev-2836-implement-atmos-validate-editorconfig
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/editor_config.go (2)
17-25: Consider adding documentation for exported variables.Adding documentation for these variables would improve code maintainability and help other developers understand their purpose.
var ( + // editorConfigVersion is the version of the editorconfig-checker being used editorConfigVersion = "v3.0.3" + // defaultConfigFilePath is the default path to look for the .editorconfig file defaultConfigFilePath = ".editorconfig" + // initEditorConfig indicates whether to initialize a new .editorconfig file initEditorConfig bool + // currentConfig holds the current editorconfig configuration currentConfig *config.Config + // cliConfig holds the configuration from command line flags cliConfig config.Config + // configFilePath is the path to the .editorconfig file configFilePath string + // tmpExclude holds temporary exclude patterns from command line tmpExclude string )
69-105: Consider breaking down the main logic into smaller functions.The
runMainLogicfunction handles multiple responsibilities. Consider extracting the file processing and validation logic into separate functions for better maintainability.+func processFiles(config config.Config) ([]string, error) { + filePaths, err := files.GetFiles(config) + if err != nil { + return nil, err + } + return filePaths, nil +} + +func validateFiles(filePaths []string, config config.Config) error { + errors := validation.ProcessValidation(filePaths, config) + errorCount := er.GetErrorCount(errors) + if errorCount != 0 { + er.PrintErrors(errors, config) + return fmt.Errorf("%d errors found", errorCount) + } + return nil +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/editor_config.go(1 hunks)examples/quick-start-simple/.editorconfig(1 hunks)tests/test_cases.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/quick-start-simple/.editorconfig
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_cases.yaml
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/editor_config.go
122-122: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
🔇 Additional comments (3)
cmd/editor_config.go (3)
60-60: Handle potential Parse() errors.
The error from currentConfig.Parse() is silently ignored. Consider handling potential parsing errors.
133-150: Support flag configuration in atmos.yaml.
These flags should be configurable via atmos.yaml for better consistency with other Atmos commands.
122-122: 🛠️ Refactor suggestion
Use constant format string in logger call.
The logger expects a format string. Use a format string to avoid the "non-constant format string" warning.
- config.Logger.Output(editorConfigVersion)
+ config.Logger.Output("%s", editorConfigVersion)Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
122-122: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/editor_config.go (1)
37-37: Consider checking the error fromcmd.Help()
While it’s common to ignore the return forcmd.Help(), static analysis tools flag this as an unhandled error.🧰 Tools
🪛 golangci-lint (1.62.2)
37-37: Error return value of
cmd.Helpis not checked(errcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/editor_config.go(1 hunks)pkg/schema/schema.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
cmd/editor_config.go (1)
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
🪛 golangci-lint (1.62.2)
cmd/editor_config.go
37-37: Error return value of cmd.Help is not checked
(errcheck)
173-173: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
🔇 Additional comments (6)
cmd/editor_config.go (2)
66-66: Same existing concern about ignoring parsing errors
173-173: Switch to a constant format string
This logger invocation can raise “non-constant format string” warnings in some lint checks. Consider using a format method (e.g. config.Logger.Outputf("%s", editorConfigVersion)).
🧰 Tools
🪛 golangci-lint (1.62.2)
173-173: printf: non-constant format string in call to (github.com/editorconfig-checker/editorconfig-checker/v3/pkg/logger.Logger).Output
(govet)
pkg/schema/schema.go (4)
35-35: Good addition of the Validate field
The extension of the configuration schema is beneficial. Ensure you have corresponding documentation or comments explaining its usage.
42-44: Validate struct creation looks clean
No issues are spotted. This struct helps group editorconfig validation settings neatly.
46-56: New EditorConfig struct
Everything appears consistent with the rest of the configuration. If needed, add docstrings for clarity.
58-66: DisabledChecks struct is well-defined
This offers a flexible way to toggle checks. Looks good for maintainability.
| @@ -17,5 +17,4 @@ stacks: | |||
| name_pattern: "{stage}" | |||
|
|
|||
| logs: | |||
| file: "/dev/stderr" | |||
There was a problem hiding this comment.
I suggest removing this from quick start simple
Especially because this config causes all the logs to direct towards error and a quick start guide should not do that.
Of course we can document and give a seperate example (which i found many already in the repo) for this behavior.
There was a problem hiding this comment.
The point is the logs should not go to stdout. The logs.level should control what is emitted.
I recommend sharing an example of the problem, before proposing the solution.
There was a problem hiding this comment.
Based on the code in the following two places;
https://github.com/cloudposse/atmos/blob/main/pkg/logger/logger.go#L61-L98
https://github.com/cloudposse/atmos/blob/main/pkg/utils/log_utils.go#L106-L143
The log emitted is not based on the log level if we set the log file to stderr or stdout.
This caused my code with u.LogInfo to be written in stderr terminal.
I assumed this is some required behaviour given that the file path were prefixed with /dev
although in hindsight it seems why we would require this code at all?
Log level is all we should be needing.
Just FYI, I think due to above behavior we are using empty atmosConfig here; https://github.com/cloudposse/atmos/blob/main/cmd/cmd_utils.go#L170-L199
Though this would work in cases where user did not set a file for log, it won't if he sets a file for log because using empty config would not output the logs to the file specified by user in the config
There was a problem hiding this comment.
in atmos.yaml we have this config for logs
logs:
# Can also be set using 'ATMOS_LOGS_FILE' ENV var, or '--logs-file' command-line argument
# File or standard file descriptor to write logs to
# Logs can be written to any file or any standard file descriptor, including `/dev/stdout`, `/dev/stderr` and `/dev/null`
file: "/dev/stderr"
# Supported log levels: Trace, Debug, Info, Warning, Off
# Can also be set using 'ATMOS_LOGS_LEVEL' ENV var, or '--logs-level' command-line argument
level: Info
logs.file is not the same as logs.level.
logs.file configures where to write the logs (/dev/stderr by default).
logs.level configures the verbosity of the logs (Trace, Debug, Info)
Log level is all we should be needing.
no, we need both logs.file and logs.level.
the user should be able to redirect logs (at any level) to a file or any other file descriptor
There was a problem hiding this comment.
@aknysh
Thank you for the explanation regarding logs.file and logs.level. I agree that both are necessary for flexibility. However, my concern is about the specific behavior when the log destination (logs.file) is set to /dev/stderr or /dev/stdout.
Currently, based on the code: https://github.com/cloudposse/atmos/blob/main/pkg/utils/log_utils.go#L113-L117
Logs are written to stderr or stdout directly, regardless of the log level set in logs.level.
This behavior causes logs (e.g., Info) to appear in stderr, which might not align with user expectations.
Additionally:
In cmd_utils.go L170-L199, the use of an empty atmosConfig works fine when no file is specified for logs.file. However, if a user configures a specific file for logs.file, this logic may not correctly redirect logs to the specified file.
Could you confirm if this is the intended behavior, or would it make sense to update the implementation to ensure that:
Logs respect both logs.file and logs.level settings, even when writing to stderr or stdout.
cmd_utils.go handles configurations with logs.file more robustly?
How to reproduce the issue:
Run atmos validate editorconfig > stdout.log 2> stderr.log With the log file set to /dev/stderr in atmos.yaml
What will happen?
You will see even the logs that are info/output of the library are published to stderr.log
This is the reason the test case fails because all logs should not go to stderr.log info logs should go stdout.log
There was a problem hiding this comment.
Thanks for the write up! If this is the current behavior, then it is indeed wrong.
For example, if logs.file is set to /dev/stderr and logs.level is Debug, I would only expect to see {Info, Warn, Error, and Debug} messages.
If logs.file is set to /dev/stderr and logs.level is Info, I would only expect to see {Info} messages.
If logs.file is set to /dev/stderr and logs.level is Trace, I would expect to see every single log level message.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cmd/editor_config.go (4)
19-26: Consider renamingtmpExcludefor clarity.The variable name
tmpExcludedoesn't clearly convey its purpose as a CLI-provided exclude pattern that gets merged into the configuration.- tmpExclude string + cliExcludePattern string
36-39: Handle potential Help() error.The
cmd.Help()function returns an error that should be handled.if cliConfig.Help { - cmd.Help() - os.Exit(0) + if err := cmd.Help(); err != nil { + u.LogErrorAndExit(atmosConfig, err) + } + os.Exit(0) }🧰 Tools
🪛 golangci-lint (1.62.2)
37-37: Error return value of
cmd.Helpis not checked(errcheck)
74-117: Consider simplifying config replacement logic.The repetitive if-blocks for config replacement could be simplified using reflection or struct field mapping for similar fields, especially in the
Disablesection.Consider extracting the
Disablefield handling into a separate function:func replaceDisableConfig(cliConfig *config.Config, atmosConfig schema.AtmosConfiguration) { disableFields := map[string]struct{ cli *bool atmos bool }{ "TrimTrailingWhitespace": {&cliConfig.Disable.TrimTrailingWhitespace, atmosConfig.Validate.EditorConfig.Disable.TrimTrailingWhitespace}, // ... other fields } for _, field := range disableFields { if field.atmos { *field.cli = field.atmos } } }
146-146: Remove newline prefix from error message.The error message starts with a newline character, which is unnecessary as the logging framework handles line formatting.
- u.LogErrorAndExit(atmosConfig, fmt.Errorf("\n%d errors found", errorCount)) + u.LogErrorAndExit(atmosConfig, fmt.Errorf("%d errors found", errorCount))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/editor_config.go(1 hunks)examples/quick-start-simple/atmos.yaml(0 hunks)tests/test_cases.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- examples/quick-start-simple/atmos.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_cases.yaml
🧰 Additional context used
📓 Learnings (1)
cmd/editor_config.go (1)
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
🪛 golangci-lint (1.62.2)
cmd/editor_config.go
37-37: Error return value of cmd.Help is not checked
(errcheck)
🔇 Additional comments (4)
cmd/editor_config.go (4)
1-17: LGTM! Clean import organization.
The imports are well-organized and properly aliased to avoid naming conflicts.
44-72: LGTM! Solid configuration initialization.
The function follows established error handling patterns and correctly handles configuration initialization.
153-163: LGTM! Solid version check implementation.
The version check is well-implemented with clear error messages and proper handling of edge cases.
166-182: Consider previous feedback about flags.
The version flag and atmos.yaml configuration suggestions from previous reviews are still applicable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
atmos.yaml (2)
97-111: Consider adding format validation.The configuration is well-structured with clear documentation. However, the
formatoption could benefit from validation to ensure only "default" or "json" are accepted.Consider adding validation in the schema to restrict the format values:
format: type: string enum: ["default", "json"] default: "default"
113-131: Strong defaults that promote code quality!The disable section is well-structured with all checks enabled by default (disable: false). This is a secure default that encourages consistent code formatting across the team. Each option is clearly documented and provides granular control when needed.
The default configuration (all checks enabled) will help maintain consistent code quality across teams. Consider documenting these defaults in your team's coding standards documentation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
atmos.yaml(1 hunks)
🔇 Additional comments (1)
atmos.yaml (1)
94-96: LGTM! Clean introduction of the validate section.
The section is well-structured and properly documented, following the established pattern in the configuration file.
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
… of https://github.com/cloudposse/atmos into feature/dev-2836-implement-atmos-validate-editorconfig
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
mergify
bot
added
triage
|
💥 This pull request now has conflicts. Could you fix it @samtholiya? 🙏 |
|
Add the docs as a subpage to: https://atmos.tools/cli/commands/validate/usage |
tests/test_cases.yaml
Outdated
| - "editorconfig" | ||
| expect: | ||
| stdout: | ||
| - "EditorConfig Checker CLI Version: v3.0.3" |
There was a problem hiding this comment.
| - "EditorConfig Checker CLI Version: v3.0.3" |
There was a problem hiding this comment.
This is out dated. It was already removed. Not sure why you got the old diff
| @@ -86,6 +86,11 @@ jobs: | |||
| timeout-minutes: 15 | |||
| runs-on: ${{ matrix.flavor.os }} | |||
| steps: | |||
| - name: Set Git Preferences for windows | |||
There was a problem hiding this comment.
Why do we need these if we do not commit any files?
There was a problem hiding this comment.
Add a comment explaining why this is needed
There was a problem hiding this comment.
Will add the comment. It is related to how git clones the repo.
| @@ -17,5 +17,4 @@ stacks: | |||
| name_pattern: "{stage}" | |||
|
|
|||
| logs: | |||
| file: "/dev/stderr" | |||
There was a problem hiding this comment.
The point is the logs should not go to stdout. The logs.level should control what is emitted.
I recommend sharing an example of the problem, before proposing the solution.
… of https://github.com/cloudposse/atmos into feature/dev-2836-implement-atmos-validate-editorconfig
issue: https://linear.app/cloudposse/issue/DEV-2836/implement-atmos-validate-editorconfig
what
why
references
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores