-
Notifications
You must be signed in to change notification settings - Fork 2k
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
with 1.13.0, breaking change has been introduced on yaml #1431
Comments
👋 Thanks for reporting! A maintainer will take a look at your issue shortly. 👀 In the meantime: We are working on Viper v2 and we would love to hear your thoughts about what you like or don't like about Viper, so we can improve or fix those issues. ⏰ If you have a couple minutes, please take some time and share your thoughts: https://forms.gle/R6faU74qPRPAzchZ9 📣 If you've already given us your feedback, you can still help by spreading the news, https://twitter.com/sagikazarmark/status/1306904078967074816 Thank you! ❤️ |
Thanks for reporting @jerome-laforge ! Give me some time to debug what may have caused this, but I have a gut feeling it's an encoding library issue. |
currently, I still stuck on 1.12. So nothing is urgent :) |
This change was introduced in #1387 as a bug fix. Unfortunately, the behavior so far was the result of a bug that was fixed in that PR (Viper does not support case-sensitivity at the moment). As such, this is not covered by the backward compatibility promise and it's not something we can revert, sorry. |
That means this fix will be included in 1.14? |
Ah I misunderstand. This bug will never fix because the previous behavior it was the bug. Do I understand correctly? :( |
Yep, the previous behavior was the result of a bug which is now fixed. |
Ok, I have to fix on my side. Thx for the clarification. |
Well, case sensitivity has been long debated and requested as a feature, but ultimately it doesn't fit the primary goals of the library. Most of it was already case sensitive before this release. The only reason it worked for you because you used an array. In every other scenario, it would be converted to lower case even before the last release. See #1014 for more details. |
ok, thx again for clarification, I will fix on business side in our application |
@sagikazarmark it's a problem for golangci-lint: we have to send "raw configuration data" to a linter, this linter has some hardcoded settings in camelCase. The way to handle this linter configuration is already complex because we have to extract a "raw" config, and then encode it to TOML, decode it from TOML (it's related to the difference in raw map types between YAML and TOML parsers), then "normalize" the configuration, and finally convert this configuration to linting rules. (Yes it's a lot of conversions) I can understand the point of view of Viper but for golangci-lint it's a regression. |
@ldez Sorry about the inconvenience! Please keep in mind this particular change is only related to to configuration in lists (slices). The rest of the confguration was always case-insensitive. We might be able to relax this rule once we reach v2, but that's not gonna happen over night. One potential solution I can imagine to resolve the situation is decoding the config into a struct and then re-encoding it: type Config struct {
SomeKey string `mapstructure:"someKey" yaml:"someKey"
} Decoding the above struct from Viper and then manually YAML encoding it should result in the desired scenario. As you say, there are already lots of conversions involved so it may not be as simple as above. Also, it requires you to basically copy the entire configuration struct from the linter which could be problematic if it's longer than a few fields. But I'd start thinking along this line. I really hope this helps. If not, I'd be happy to help brainstorming an alternative solution. Let me know what you think! |
Just to share with you, the linter configuration is a slice (sadly not a map) of "neutral" configuration that contains a slice of interface. So it's impossible to create a struct with the exhaustive configuration. type ReviveSettings struct {
// ...
Rules []struct {
Name string
Arguments []interface{}
// ...
}
// ...
} A configuration example: linters-settings:
revive:
rules:
- name: add-constant
severity: warning
disabled: false
arguments:
- maxLitCount: "3"
allowStrs: '""'
allowInts: "0,1,2"
allowFloats: "0.0,0.,1.0,1.,2.0,2."
- name: string-format
severity: warning
disabled: false
arguments:
- - 'core.WriteError[1].Message'
- '/^([^A-Z]|$)/'
- must not start with a capital letter
- - 'fmt.Errorf[0]'
- '/(^|[^\.!?])$/'
- must not end in punctuation
- - panic
- '/^[^\n]*$/'
- must not contain line breaks |
From what I can tell the only affected rules are the ones where an argument is actually a map (because the keys become lower-cased). Based on this document that's two rules (and five arguments total):
One potential solution is writing a post processing algorithm, that goes through the rules and updates the argument names (there are only 5) based on a map. I realize this is not ideal (with every update you have to make sure that new rules are processed), but it's a solution. An alternative I can imagine is making the arguments section a raw string instead of an array: linters-settings:
revive:
rules:
- name: add-constant
severity: warning
disabled: false
arguments: |
- maxLitCount: "3"
allowStrs: '""'
allowInts: "0,1,2"
allowFloats: "0.0,0.,1.0,1.,2.0,2." (Notice the Finally, you could combine the above two to avoid breaking changes: add a map for the existing rules, and require using raw yaml for new rules added to revive. I'm trying to come up with a solution for Viper to prevent processing certain values based on some sort of escape mechanism or configuration. For example: v := viper.New(
viper.EscapePath("linters-settings.revive.rules"), // Could be IgnorePath or something similar as well
) The problem with the above is that Viper would essentially handle It seems to me that the quickest solution at the moment is option 1: post-processing keys for those two rules. |
yes it was just to explain you the complexity of the configuration.
Yes currently I don't find other solution but this solution will be complex to maintain because revive doesn't have a concrete configuration, so for each update we will have to check inside the code if a new option has been added.
I don't want to follow this way because it's impossible to use the JSON schema validation on this kind a of type. |
Thank you for trying to help me to find solution ❤️ |
@ldez @jerome-laforge due to popular demand, I'm going to allow reverting back to the previous behavior using a go tag. |
Preflight Checklist
Viper Version
1.13.0
Go Version
1.19
Config Source
Defaults
Format
YAML
Repl.it link
No response
Code reproducing the issue
with config :
Expected Behavior
with 1.12.0, seq equals
[{[{map[FR:{false}]} {map[EN:{false} FR:{true}]}]}]
with 1.13.0, seq equals
[{[{map[fr:{false}]} {map[en:{false} fr:{true}]}]}]
Actual Behavior
expected we have the same case
FR
notfr
for the key mapSteps To Reproduce
No response
Additional Information
No response
The text was updated successfully, but these errors were encountered: