Skip to content
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

Closed
ppalaga opened this issue Nov 2, 2017 · 7 comments
Closed

Add a test for "unset" value #12

ppalaga opened this issue Nov 2, 2017 · 7 comments
Assignees
Labels

Comments

@ppalaga
Copy link
Contributor

ppalaga commented Nov 2, 2017

Citing from [1]:

For any property, a value of unset is to remove the effect of that property, even if it has been set before.

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

@xuhdev xuhdev added the bug label Nov 3, 2017
@xuhdev xuhdev self-assigned this Nov 7, 2017
@xuhdev
Copy link
Member

xuhdev commented Nov 7, 2017

I will look into this. Thanks!

@xuhdev
Copy link
Member

xuhdev commented Nov 13, 2017

@ppalaga The reason that we do not have specific tests for unset is that unset is really treated as a regular value for the core library -- but it is formally defined in the documentation such that plugins should ignore properties with the unset value. Since it is nothing special for the core library, there is no need to have a special test for that.

@xuhdev xuhdev closed this as completed Nov 13, 2017
@xuhdev xuhdev added wontfix and removed bug labels Nov 13, 2017
@ppalaga
Copy link
Contributor Author

ppalaga commented Nov 13, 2017

plugins should ignore properties with the unset value

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 unset value from the result set.

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.

@xuhdev
Copy link
Member

xuhdev commented Nov 13, 2017

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.

ppalaga added a commit to ppalaga/editorconfig-core-test that referenced this issue Nov 13, 2017
@ppalaga
Copy link
Contributor Author

ppalaga commented Nov 13, 2017

@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 ec4j because we believe it suits our needs well. But at the same time, we want to ensure that our lib accepts all allowed values including unset. This is the reason we'd very much appreciate to get a set of tests as submitted via #16

@xuhdev
Copy link
Member

xuhdev commented Nov 14, 2017

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 unset in the core library output is a bit dangerous: What if some editors defined some custom properties and they see unset as a real thing? Or, in the past, they have treated unset differently? In addition, by having the core library evolves from not treating unset differently to treating unset differently is confusing by itself, not to mention we have multiple core libraries and it's hard to synchronize all of them.

@ppalaga
Copy link
Contributor Author

ppalaga commented Nov 14, 2017

I'm not sure whether you understood what I've said.

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 charset=utf-8 above charset=unset in the file system hierarchy will return charset=unset - that's exactly what core-c and core-py are doing. Such tests are fully in line with what you say and I hope you have nothing against merging the proposed patch.

ppalaga added a commit to ppalaga/editorconfig-core-test that referenced this issue Nov 17, 2017
xuhdev pushed a commit that referenced this issue Nov 17, 2017
kini added a commit to kini/editorconfig-emacs that referenced this issue Jul 19, 2020
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
kini added a commit to kini/editorconfig-emacs that referenced this issue Jul 27, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants