-
Notifications
You must be signed in to change notification settings - Fork 22
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 test for "unset" value #12
Comments
I will look into this. Thanks! |
@ppalaga The reason that we do not have specific tests for |
Thanks for the explanation. I must say I do not find this decision fortunate, because in this way we make the life more complicated to the plugins trough requiring from them to handle this special value. I think that it would be much less error prone for all parties to require from the property filtering facility (such as editorconfig binary) to simply remove properties having But OK, I am ready to take this as your decision that you maybe have your reasons for and that you maybe cannot change for backwards compatibility reasons. In any case, this decision should be checked by the tests so that implementations cannot make improper assumptions about the range of the possible values for boolean and integer properties. I'd propose a PR that will add such tests if you do not mind. |
The core library actually does very little, and this is for a reason. (a) If there is any new property/new proposed and added to the specification, only plugins need to be updated -- there is no need to update the core. (b) Assuming dynamic linking, if the format itself changes (glob for example), only the core library need to be updated. The reason for (a) is very important: Linux users often have a stable core library installed, but they can frequently update editors. We also don't want to confuse plugin developers by having cores treating unset differently, and that's why we do not wanna have tests specifically on unset. |
@xuhdev all what you say is fine and valid for how core-c and core-py were designed. However, other editorconfig implementations may choose to implement more semantics, by e.g. parsing the "widely accepted" properties as their proper (boolean, integer, ...) types. We do this in |
I'm not sure whether you understood what I've said. The core libraries treat almost all properties and values as the same: no matter they are defined in the specification or not. This consistent behavior gives plugins a lot of flexibility: they can define their editor specific properties and values, without the need to touch the core libraries. To remove |
I'm not sure whether you understood what I've said :) I am not proposing to change the behavior of core-c and core-py. I am rather proposing to add tests that firm up the current behavior of core-c and core-py so that other libs have a chance to see whether they comply or not. You can clearly see that if you look at the proposed patch: ppalaga@dd28435#diff-50d5f8f9a4351930d5c5cbe948b718b4R89 E.g. having |
According to [the editorconfig specification][1], > For any pair, a value of `unset` removes the effect of that pair, even if it > has been set before. For example, add `indent_size = unset` to undefine the > `indent_size` pair (and use editor defaults). Yet, the test suite for editorconfig *cores* [indicates][2] that an editorconfig shouldn't ignore values of "unset", and should return them as-is. Therefore it's up to editorconfig *plugins* to discard values of "unset" at the point of use. Before this commit, the only place in the code where the value "unset" was checked for was in `editorconfig-set-major-mode-from-ext`, whereas in other places, such as `editorconfig-set-indentation`, it was not checked. After this commit, values of "unset" are immediately removed from the properties hashtable before it is used to do anything in `editorconfig-apply`. This ensures that properties with value "unset" will be treated indistinguishably from unspecified properties. This fixes the behavior shown in this gist: https://gist.github.com/kini/560673dbff4b8e42bb40a82a8ca6e5b1 [1]: https://editorconfig-specification.readthedocs.io/en/latest/#supported-pairs [2]: editorconfig/editorconfig-core-test#12
According to [the editorconfig specification][1], > For any pair, a value of `unset` removes the effect of that pair, even if it > has been set before. For example, add `indent_size = unset` to undefine the > `indent_size` pair (and use editor defaults). Yet, the test suite for editorconfig *cores* [indicates][2] that an editorconfig shouldn't ignore values of "unset", and should return them as-is. Therefore it's up to editorconfig *plugins* to discard values of "unset" at the point of use. Before this commit, the only place in the code where the value "unset" was checked for was in `editorconfig-set-major-mode-from-ext`, whereas in other places, such as `editorconfig-set-indentation`, it was not checked. After this commit, values of "unset" are immediately removed from the properties hashtable before it is used to do anything in `editorconfig-apply`. This ensures that properties with value "unset" will be treated indistinguishably from unspecified properties. This fixes the behavior shown in this gist: https://gist.github.com/kini/560673dbff4b8e42bb40a82a8ca6e5b1 [1]: https://editorconfig-specification.readthedocs.io/en/latest/#supported-pairs [2]: editorconfig/editorconfig-core-test#12
Citing from [1]:
Searching for
unset
in the core-test project returns zero matches.Expected: there should be at least one test for each of the widely accepted properties that unsets a previously set value. This is i.a. important to make sure that handlers for properties that look like boolean or integer at the first sight can handle
unset
too.[1] https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#current-universal-properties
The text was updated successfully, but these errors were encountered: