-
-
Notifications
You must be signed in to change notification settings - Fork 117
Support the --config flag as an array of strings for atmos validate editorconfig
#1173
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
Support the --config flag as an array of strings for atmos validate editorconfig
#1173
Conversation
8ed4af5 to
2d34d25
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1173 +/- ##
==========================================
+ Coverage 28.29% 28.34% +0.05%
==========================================
Files 182 182
Lines 21284 21284
==========================================
+ Hits 6022 6034 +12
+ Misses 14312 14280 -32
- Partials 950 970 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThe changes update configuration file path handling from a single string to multiple strings. Modifications occur in the core command handling, schema definitions, CLI flag documentation, and testing. The persistent flag now accepts a slice of strings with defined defaults, and related functions have been adjusted to support multiple configuration file paths. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant Cmd as ValidateEditorconfig Command
participant Config as Configuration Handler
participant Schema as EditorConfig Schema
User->>Cmd: Provide --config flag values
Cmd->>Config: Call initializeConfig()
alt ConfigFilePaths provided
Config->>Config: Append provided values to configPaths
else
Config->>Config: Append default config file names to configPaths
end
Config->>Schema: Pass configPaths to EditorConfig struct
Schema-->>Cmd: Return updated configuration
Cmd-->>User: Execute editorconfig validation with new paths
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/validate_editorconfig.go (1)
78-80: Consider adding explicit nil check for ConfigFilePaths.While checking length works for nil slices in Go, adding an explicit nil check would improve code clarity and prevent potential issues if implementation details change in the future.
- if !cmd.Flags().Changed("config") && len(atmosConfig.Validate.EditorConfig.ConfigFilePaths) > 0 { + if !cmd.Flags().Changed("config") && atmosConfig.Validate.EditorConfig.ConfigFilePaths != nil && len(atmosConfig.Validate.EditorConfig.ConfigFilePaths) > 0 { configFilePaths = atmosConfig.Validate.EditorConfig.ConfigFilePaths }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 79-79: cmd/validate_editorconfig.go#L79
Added line #L79 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/validate_editorconfig.go(4 hunks)cmd/validate_editorconfig_test.go(1 hunks)pkg/schema/schema.go(1 hunks)tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_validate_editorconfig_help.stdout.golden(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
cmd/validate_editorconfig.go
[warning] 54-54: cmd/validate_editorconfig.go#L54
Added line #L54 was not covered by tests
[warning] 79-79: cmd/validate_editorconfig.go#L79
Added line #L79 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (7)
cmd/validate_editorconfig_test.go (1)
5-12: Good addition of a test function for config initialization.This test ensures the
initializeConfigfunction works with the editorConfigCmd. The comments clearly explain why this test is minimal and reference ticket DEV-3094 for future test coverage improvements.pkg/schema/schema.go (1)
173-177: Proper update from single config to multiple configs.The field has been appropriately changed from
ConfigFilePath stringtoConfigFilePaths []stringwhile maintaining the same struct tags. This change aligns with the PR's objective to support an array of strings for editorconfig configuration.tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden (1)
11-13: Help text correctly updated to show stringSlice type with defaults.The help text now accurately reflects that the
--configflag accepts multiple values (stringSlice) with appropriate default values. This ensures users understand the new capability to specify multiple configuration files.tests/snapshots/TestCLICommands_atmos_validate_editorconfig_help.stdout.golden (1)
11-13: Help text correctly updated to show stringSlice type with defaults.The help text for this alternative command format has been consistently updated to reflect the new
--configflag type and default values, maintaining uniformity across the CLI documentation.cmd/validate_editorconfig.go (3)
27-27: Variable type updated to support multiple config files.This change correctly updates the configFilePath variable from a string to a slice of strings, aligning with the PR's objective to support an array of strings for editorconfig validation.
182-182: Flag updated to accept multiple configuration paths.The StringSliceVar correctly replaces StringVar, and defaultConfigFileNames is appropriately used as the default value. This change maintains backward compatibility while supporting the new functionality.
53-57:❓ Verification inconclusive
Initialization logic updated for multiple config files.
The logic correctly handles both cases - when configFilePaths is empty (using defaults) and when it contains paths (using provided values). This ensures backward compatibility while supporting the new array structure.
🏁 Script executed:
#!/bin/bash # Verify test coverage for this code segment grep -r "func TestInitialize" cmd/Length of output: 36
Action: Verify test coverage for editorconfig initialization logic
The updated initialization code in
cmd/validate_editorconfig.go(lines 53–57) correctly handles both empty and non-emptyconfigFilePathsby respectively appending the default configuration names or the provided paths. This logic appropriately maintains backward compatibility while extending functionality.However, our recent verification attempt using:
grep -r "func TestInitialize" cmd/did not yield any output.
Please verify manually or add tests to ensure that:
- The default configuration branch (when
configFilePathsis empty) is adequately tested.- The provided configuration branch (when
configFilePathsis non-empty) is covered by tests.If tests already exist under a different name (e.g.,
TestValidateEditorConfig), please confirm they comprehensively handle both cases. Otherwise, consider adding these tests.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 54-54: cmd/validate_editorconfig.go#L54
Added line #L54 was not covered by tests
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.
@samtholiya please address a few comments
also, please update the docs https://atmos.tools/cli/commands/validate/editorconfig/
Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/docs/cli/commands/validate/validate-editorconfig.mdx (1)
41-57: Optional: Use Typographic Quotes for Improved Readability: Static analysis suggests replacing the plain quotes around the default file names (".editorconfig", ".editorconfig-checker.json", ".ecrc") with typographic quotation marks. This is a minor nitpick that could enhance the visual polish of the documentation.🧰 Tools
🪛 LanguageTool
[typographical] ~43-~43: Consider using a typographic opening quote here.
Context: ...ath to the configuration file (default: ".editorconfig", ".editorconfig-checker.j...(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic close quote here.
Context: ...figuration file (default: ".editorconfig", ".editorconfig-checker.json", ".ecrc")...(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic opening quote here.
Context: ...uration file (default: ".editorconfig", ".editorconfig-checker.json", ".ecrc") ...(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic close quote here.
Context: ...itorconfig", ".editorconfig-checker.json", ".ecrc") | |no | |...(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic opening quote here.
Context: ...rconfig", ".editorconfig-checker.json", ".ecrc") | |no | |`--...(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic close quote here.
Context: ...g", ".editorconfig-checker.json", ".ecrc") | |no | |`--disabl...(EN_QUOTES)
[typographical] ~44-~44: Consider using typographic quotation marks here.
Context: ... |Disable end-of-line check (default "false") ...(EN_QUOTES)
[typographical] ~45-~45: Consider using typographic quotation marks here.
Context: ... |Disable indent size check (default "false") ...(EN_QUOTES)
[typographical] ~46-~46: Consider using typographic quotation marks here.
Context: ... |Disable indentation check (default "false") ...(EN_QUOTES)
[typographical] ~47-~47: Consider using typographic quotation marks here.
Context: ... |Disable final newline check (default "false") ...(EN_QUOTES)
[typographical] ~48-~48: Consider using typographic quotation marks here.
Context: ...|Disable max line length check (default "false") ...(EN_QUOTES)
[typographical] ~49-~49: Consider using typographic quotation marks here.
Context: ...able trailing whitespace check (default "false") ...(EN_QUOTES)
[typographical] ~50-~50: Consider using typographic quotation marks here.
Context: ...w which files would be checked (default "false") ...(EN_QUOTES)
[typographical] ~52-~52: Consider using typographic quotation marks here.
Context: ...he output format: default, gcc (default "default") ...(EN_QUOTES)
[typographical] ~53-~53: Consider using typographic quotation marks here.
Context: ... |help for editorconfig (default "false") ...(EN_QUOTES)
[typographical] ~54-~54: Consider using typographic quotation marks here.
Context: ... |Ignore default excludes (default "false") ...(EN_QUOTES)
[typographical] ~55-~55: Consider using typographic quotation marks here.
Context: ...eates an initial configuration (default "false") ...(EN_QUOTES)
[typographical] ~56-~56: Consider using typographic quotation marks here.
Context: ... |Don't print colors (default "false") ...(EN_QUOTES)
[typographical] ~57-~57: Consider using typographic quotation marks here.
Context: ... |Print the version number (default "false") ...(EN_QUOTES)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/docs/cli/commands/validate/validate-editorconfig.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/cli/commands/validate/validate-editorconfig.mdx
[typographical] ~43-~43: Consider using a typographic opening quote here.
Context: ...ath to the configuration file (default: ".editorconfig", ".editorconfig-checker.j...
(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic close quote here.
Context: ...figuration file (default: ".editorconfig", ".editorconfig-checker.json", ".ecrc")...
(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic opening quote here.
Context: ...uration file (default: ".editorconfig", ".editorconfig-checker.json", ".ecrc") ...
(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic close quote here.
Context: ...itorconfig", ".editorconfig-checker.json", ".ecrc") | |no | |...
(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic opening quote here.
Context: ...rconfig", ".editorconfig-checker.json", ".ecrc") | |no | |`--...
(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic close quote here.
Context: ...g", ".editorconfig-checker.json", ".ecrc") | |no | |`--disabl...
(EN_QUOTES)
[typographical] ~44-~44: Consider using typographic quotation marks here.
Context: ... |Disable end-of-line check (default "false") ...
(EN_QUOTES)
[typographical] ~45-~45: Consider using typographic quotation marks here.
Context: ... |Disable indent size check (default "false") ...
(EN_QUOTES)
[typographical] ~46-~46: Consider using typographic quotation marks here.
Context: ... |Disable indentation check (default "false") ...
(EN_QUOTES)
[typographical] ~47-~47: Consider using typographic quotation marks here.
Context: ... |Disable final newline check (default "false") ...
(EN_QUOTES)
[typographical] ~48-~48: Consider using typographic quotation marks here.
Context: ...|Disable max line length check (default "false") ...
(EN_QUOTES)
[typographical] ~49-~49: Consider using typographic quotation mar
8000
ks here.
Context: ...able trailing whitespace check (default "false") ...
(EN_QUOTES)
[typographical] ~50-~50: Consider using typographic quotation marks here.
Context: ...w which files would be checked (default "false") ...
(EN_QUOTES)
[typographical] ~52-~52: Consider using typographic quotation marks here.
Context: ...he output format: default, gcc (default "default") ...
(EN_QUOTES)
[typographical] ~53-~53: Consider using typographic quotation marks here.
Context: ... |help for editorconfig (default "false") ...
(EN_QUOTES)
[typographical] ~54-~54: Consider using typographic quotation marks here.
Context: ... |Ignore default excludes (default "false") ...
(EN_QUOTES)
[typographical] ~55-~55: Consider using typographic quotation marks here.
Context: ...eates an initial configuration (default "false") ...
(EN_QUOTES)
[typographical] ~56-~56: Consider using typographic quotation marks here.
Context: ... |Don't print colors (default "false") ...
(EN_QUOTES)
[typographical] ~57-~57: Consider using typographic quotation marks here.
Context: ... |Print the version number (default "false") ...
(EN_QUOTES)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Lint (golangci)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: website-deploy-preview
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
website/docs/cli/commands/validate/validate-editorconfig.mdx (1)
41-43: Update Flag Type for Config: The configuration flag is still labeled as--config string, but given the PR’s objective you should update it to reflect the new array-of-strings type (e.g.,--config stringSlice). This change will ensure consistency between the documentation and the underlying implementation.
[suggest_nitpick, request_verification]🧰 Tools
🪛 LanguageTool
[typographical] ~43-~43: Consider using a typographic opening quote here.
Context: ...ath to the configuration file (default: ".editorconfig", ".editorconfig-checker.j...(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic close quote here.
Context: ...figuration file (default: ".editorconfig", ".editorconfig-checker.json", ".ecrc")...(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic opening quote here.
Context: ...uration file (default: ".editorconfig", ".editorconfig-checker.json", ".ecrc") ...(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic close quote here.
Context: ...itorconfig", ".editorconfig-checker.json", ".ecrc") | |no | |...(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic opening quote here.
Context: ...rconfig", ".editorconfig-checker.json", ".ecrc") | |no | |`--...(EN_QUOTES)
[typographical] ~43-~43: Consider using a typographic close quote here.
Context: ...g", ".editorconfig-checker.json", ".ecrc") | |no | |`--disabl...(EN_QUOTES)
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.
@samtholiya please address the comment
--config flag as an array of strings for atmos validate editorconfig
|
These changes were released in v1.169.0. |
what
--configflag as an array of strings foratmos validate editorconfigwhy
configwould be an array of string at the root. And this should be reflected invalidate editorconfigreferences
Summary by CodeRabbit
New Features
Documentation