-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add a way to disable absolute-first option explicitly #1664
Conversation
2 similar comments
Any news on this? It's literally a one-liner change. |
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.
Sorry it took me so long to review. We'd need test cases for this in order to accept it.
I don't have much experience in writing tests. |
Something like this? |
Such a test cannot exist.
This option doesn't actually do anything by itself, it's purely structural, intended for reset to default behavior. In particular, if a config you source from sets 'absolute-first', this options allows you to actually unset it. Before, we did it by passing through an empty string to options, but the new consistency checks on this options prevent you from doing that, so I set out to fix that.
I honestly think it's a bug, not an enhancement, since it's the default validation that is overly restrictive, there's no new functionality here.
It could have as well have been an empty string, I just figured explicitly writing you want to turn it off might be nicer to look at. Perhaps this only confuses people, and I should have gone with an empty string instead.
Bottom line is, 'disable-absolute-first' doesn't have any behavior that differs from how things work by default, it's only intended to reset options if they aren't default already.
|
Thanks, I think i get it now :-) |
So, should I merge explicit 'disable-absolute-first', or is an empty string option better? I'm kinda in doubt here. Or are both fine?
|
This one's good; an empty string is super confusing. |
Fixes #1661 .
Not sure if the alternative should be just an empty string or an explicit disable option. Went for the second for now.