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

Clarify behavior of formatting (non-"root") properties at top level #8

Closed
Artoria2e5 opened this issue Sep 3, 2019 · 10 comments · Fixed by #12
Closed

Clarify behavior of formatting (non-"root") properties at top level #8

Artoria2e5 opened this issue Sep 3, 2019 · 10 comments · Fixed by #12

Comments

@Artoria2e5
Copy link
Contributor

Artoria2e5 commented Sep 3, 2019

Currently there is no mention of what these things should do in neither the simplified nor the formal version of the spec. The example files in official libraries seems to generally avoid doing so, and WebStorm seems to agree that they are in general invalid.

If the current behavior is intended to be kept, I recommend adding this to the spec:

Any property other than root MUST be located under a section to take effect. They are not otherwise recognized.

This, however, can feel counterintuitive to some. It could be more intuitive to do this (at a cost of changing quite a few libraries; we will need a versioning scheme for that):

Formatting (non-root) properties not found under a section are taken to have a section of [*]. The normal precedence rules apply.

The example should probably be changed to illustrate the precedence behavior too. Maybe define a indent under [*] and overwriting it for js will help show the point.

@Artoria2e5 Artoria2e5 changed the title Clarify behavior of file-format (non-"root") properties at top level Clarify behavior of formatting (non-"root") properties at top level Sep 3, 2019
@cxw42
Copy link
Member

cxw42 commented Sep 3, 2019

I agree with updating the spec to clarify the current behaviour. I agree that implicit [*] would be more ergonomic, but I personally think we would need to more clearly define our versioning scheme before making such a significant change! Edit Specifically, I think we would need a way for cores or plugins to provide a meaningful error message when encountering an unsupported feature. E.g., perhaps a root version property. However, that would be a separate issue.

@jednano
Copy link
Member

jednano commented Nov 5, 2019

To be clear, the EditorConfig INI parser allows you to specify any/all custom properties that you want. Some people actually use this for their own custom implementations outside of EditorConfig. root=true is the only root property that EditorConfig does anything with, but I don't know if invalid is the right word to use if a property that is not root=true is at the root level.

That said, I also very much agree that we need more clarification and less ambiguity about these things and I'm actively working on that very goal during my international travels. I hope at least we can make one step forward on this.

@xuhdev
Copy link
Member

xuhdev commented Nov 6, 2019

It doesn't sound right to me to put any other settings on root. Do we have statistics on what we do right now?

@jednano
Copy link
Member

jednano commented Nov 6, 2019

@xuhdev for the purposes of EditorConfig, you're right. That's all we would ever need or support. But for anyone outside of EditorConfig who is perhps using the config inheritance feature of EditorConfig, there's nothing stopping them from defining custom properties at the root level.

This is another reason why I think it's important to distinguish between:

  1. The actual INI parsing and only the INI parsing. No rule restrictions at root or within sections.
  2. Supporting EditorConfig-specific rules.
  3. The inheritance feature.

These are all very different things and I can see non-EditorConfig implementations that rely upon 1 & 3, but not 2.

@xuhdev
Copy link
Member

xuhdev commented Nov 6, 2019

I think whether an assignment not under any section is a matter of syntactic definition. If we allow any anything other than root outside any section, we would be essentially repeating the functionality of [*]. I don't see any reason to duplicate this functionality twice. But I agree we should make this point clear.

Currently, as far as I see, the C core does not output anything not in any sections.

@jednano
Copy link
Member

jednano commented Nov 6, 2019

You're right. It doesn't look like the js core does either.

root = true
foo = bar

[*]
qux = corge

The output was just qux=corge.

I think the INI parser should definitely record those properties at root though. Whether we do anything with them is another story.

@xuhdev
Copy link
Member

xuhdev commented Nov 6, 2019

I'm not sure whether there's any benefit to break with the existing behavior and output properties at root. I'm personally leaning toward clarifying this existing behavior rather than altering it.

@jednano
Copy link
Member

jednano commented Nov 7, 2019

To be clear, I'm not suggesting we output, change or break anything with respect to the existing EditorConfig implementation. I'm saying that the INI parsing aspect of EditorConfig can be decoupled into a separate parser that knows nothing about EditorConfig's rules and especially nothing about root=true or any of the merging strategy.

As it doesn't know anything about root=true, it doesn't make sense to impose this artificial limitation of only having one property in the default/root section. Once an EditorConfig core gets its hands on this AST provided by the parser, then it can ignore the unsupported rules or even throw validation errors at that point, if we decide to go that route.

The point is, we can keep the INI parsing as agnostic to implementation as possible and put EditorConfig-specific "business logic" into the cores instead of the INI parser itself.

@xuhdev
Copy link
Member

xuhdev commented Nov 7, 2019

OK, sounds good to me.

@jednano
Copy link
Member

jednano commented Nov 7, 2019

Now to answer the OP, I think we should stick with what he said:

Any property other than root MUST be located under a section to take effect. They are not otherwise recognized.

And don't do any of the default [*] stuff.

jednano added a commit to jednano/specification that referenced this issue Nov 9, 2019
jednano pushed a commit that referenced this issue Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants