-
Notifications
You must be signed in to change notification settings - Fork 169
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
feat: add options for import and delete normalization handling #370
feat: add options for import and delete normalization handling #370
Conversation
* add normalization helpers for services and keys
enables deletion of keys that may have been written unnormalized
3dd1120
to
86d1325
Compare
func NormalizeService(service string) string { | ||
return strings.ToLower(service) | ||
} | ||
|
||
// NormalizeKey normalizes a provided secret key to a common format | ||
func NormalizeKey(key string) string { | ||
return strings.ToLower(key) | ||
} |
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.
What's the intent in moving these to a separate package? Since these both just call strings.ToLower()
, right now it's just indirection on top of stdlib.
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.
just standardization/normalization. There are separate packages in this repo that rely on the same logic, so was attempting to DRY it out by giving it an interface
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.
🤷🏻♂️ I'd argue that 2 new methods that both lowercase a string is a worse deal than just using strings.ToLower()
when needed buuuuuuut it's not a hill I care about dying on.
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.
Totally fair - I might be overthinking the need to update how or what normalization means in the future and implemented this as a measure.
I may have also thought that the lack of a normalizing function (and just raw strings.ToLower
) may have results in missed normalization in the past, which lead me to move towards it
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.
Yeah, I can see that; if any of the other normalizion/sanitization happened in these functions, I think it'd make more sense to have them. As they're written right now, they're just 1:1 wrappers around strings.ToLower()
without any benefit for the indirection.
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.
I'm not going to block on the question I left but I did have a question.
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.
LGTM!
Description
Mitigates #160, #324, #369
Adds two forward-thinking options in lieu of implementing breaking change behavior.
utils
packagechamber import
--normalize-keys
flagchamber delete
--preserve-key
flagFor the Reviewer
The first two commits are just minor refactors. It is recommended to review by each commit to reduce styling/refactor noise.
Please excuse my Prettier running rampant on
README.md