feat(config): warn on unknown keys#1614
Conversation
floatpanebot
left a comment
There was a problem hiding this comment.
Hi @MD-Mushfiqur123! Please fix the following issues with your PR:
- Title: Is too long (49 characters). The PR title must be strictly under 40 characters.
- Body: Missing the
## What?or## Why?headings required by the PR template.
476876d to
1fc1c46
Compare
floatpanebot
left a comment
There was a problem hiding this comment.
Hi @MD-Mushfiqur123! Please fix the following issues with your PR:
- Title: Is too long (49 characters). The PR title must be strictly under 40 characters.
- Body: Missing the
## What?or## Why?headings required by the PR template.
Benchmark report — no significant changeMetrics worse: 0 · better: 0 (threshold: ±3%). benchstat outputauto-generated by benchmarks.yml |
floatpanebot
left a comment
There was a problem hiding this comment.
Hi @MD-Mushfiqur123! Please fix the following issues with your PR:
- Title: Is too long (49 characters). The PR title must be strictly under 40 characters.
- Body: Missing the
## What?or## Why?headings required by the PR template.
|
This feature should go to |
floatpanebot
left a comment
There was a problem hiding this comment.
Hi @MD-Mushfiqur123! Please fix the following issues with your PR:
- Title: Is too long (41 characters). The PR title must be strictly under 40 characters.
Formatting issues have been resolved. Thank you!
|
All CI checks are passing now. Could this please be reviewed/merged when you get a chance? Thanks! |
|
@MD-Mushfiqur123 please, rebase this PR on |
abd2ce2 to
2f52179
Compare
|
@andrinoff Done! Rebased on |
andrinoff
left a comment
There was a problem hiding this comment.
This is a very unreliable implementation, this will add 2 more lists that have to be configured each time, we need a new option. This should replicate the Account and Config structs
| "accounts": true, | ||
| "disable_images": true, | ||
| "hide_tips": true, | ||
| "disable_notifications": true, | ||
| "enable_split_pane": true, | ||
| "enable_threaded": true, | ||
| "enable_detailed_dates": true, | ||
| "theme": true, | ||
| "mailing_lists": true, | ||
| "date_format": true, | ||
| "language": true, | ||
| "body_cache_threshold_mb": true, | ||
| "plugin_settings": true, |
| "id": true, | ||
| "name": true, | ||
| "email": true, | ||
| "password": true, | ||
| "service_provider": true, | ||
| "fetch_email": true, | ||
| "send_as_email": true, | ||
| "imap_server": true, | ||
| "imap_port": true, | ||
| "smtp_server": true, | ||
| "smtp_port": true, | ||
| "insecure": true, | ||
| "smime_cert": true, | ||
| "smime_key": true, | ||
| "smime_sign_by_default": true, | ||
| "pgp_public_key": true, | ||
| "pgp_private_key": true, | ||
| "pgp_key_source": true, | ||
| "pgp_pin": true, | ||
| "pgp_sign_by_default": true, | ||
| "auth_method": true, | ||
| "protocol": true, | ||
| "jmap_endpoint": true, | ||
| "pop3_server": true, | ||
| "pop3_port": true, | ||
| "catch_all": true, | ||
| "pass_cmd": true, |
34e80b9 to
938c953
Compare
|
@andrinoff Updated! Replaced the hardcoded key maps with reflection-based extraction from |
|
@andrinoff The hardcoded key lists are now replaced with |
938c953 to
f70f32e
Compare
|
@andrinoff Rebased on the latest |
andrinoff
left a comment
There was a problem hiding this comment.
Make assertion tests, these ones test nothing. For the other issue there are several solution, one of them would be to mirror rawAccount struct instead (i am not sure if that is the cleanest solution.
| func TestWarnUnknownConfigKeys(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| json string | ||
| }{ | ||
| {"no unknown keys", `{"accounts": [{"name": "test", "email": "a@b.com", "service_provider": "gmail"}], "theme": "dark"}`}, | ||
| {"empty object", `{}`}, | ||
| {"invalid json", `not json`}, | ||
| {"nested with unknown", `{"unknown_top": true, "accounts": [{"name": "test", "email": "a@b.com", "unknown_field": true}]}`}, | ||
| } | ||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| warnUnknownConfigKeys([]byte(tc.json)) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
1st of all, this test only tests, that the function will not fail, it doesnt test anything, use assertion
|
|
||
| func initKnownKeys() { | ||
| knownConfigKeys = structJSONKeys(Config{}) | ||
| knownAccountKeys = structJSONKeys(Account{}) |
There was a problem hiding this comment.
due to Account struct having Password and PGPPin as json:-, they will most likely not pass the test if config.json has them (user can use it, it will be then migrated to OS-keyrings
4cea1cf to
386d16d
Compare
|
@andrinoff Updated based on your feedback:
Please re-review when you get a chance! |
Emit a warning via log.Printf listing unrecognized config keys at startup. This helps users catch typos in their config.json. Closes floatpane#1521
Replace hardcoded knownConfigKeys and knownAccountKeys maps with dynamic extraction from Config and Account struct tags via reflection. This eliminates the maintenance burden of keeping the key lists in sync with the struct definitions.
386d16d to
71f1f82
Compare
|
Great! The logic now works as expected! Now, there are 2 aspects of this: The current implementation pushes the error to I'd suggest add an error message via After that, i think we are good to merge. |
What?
Emit a warning via log.Printf listing unrecognized config keys at startup, helping users catch typos in their config.json.
Why?
Typos in config keys are silently ignored. This change provides immediate feedback to the user.
How?
warnUnknownConfigKeysfunction that checks for unknown top-level and account-level config keysLoadConfigafter reading the fileCloses #1521