Add a test for "unset" value about editorconfig-core-test HOT 7 CLOSED

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.

from editorconfig-core-test.

I will look into this. Thanks!

from editorconfig-core-test.

@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.

from editorconfig-core-test.

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.

from editorconfig-core-test.

@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

from editorconfig-core-test.

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.

from editorconfig-core-test.

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.

from editorconfig-core-test.