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 'key containing whitespace' case #39

Open
rosingrind opened this issue Mar 23, 2023 · 8 comments
Open

Clarify 'key containing whitespace' case #39

rosingrind opened this issue Mar 23, 2023 · 8 comments

Comments

@rosingrind
Copy link

According to the spec, whitespaces in keys are not prohibited:

Key: the part before the first = (trimmed of whitespace).

Unfortunately, this behavior is not widely adopted, nor explicitly considered as a legal option. However, JetBrains seems to use this behaviour as an option when exporting .editorconfig:

[{*.http,*.rest}]
ij_http request_call_parameters_wrap = normal

This feature is documented in IDEA documentation, you can actually see implementation cases searching for code samples: https://github.com/search?q=ij_http+request_call_parameters_wrap&type=code

I'm requesting any clarification on legality and acceptability of this use case. This question was originally raised at
ec4j/ec4j#122 (comment) as this behavior may misleadingly be adopted by parser developers. If spacing is considered as allowed, please implement any tests for this in https://github.com/editorconfig/editorconfig-core-test so any other parser developers may have a chance to adapt too

@rosingrind
Copy link
Author

I'm introducing @ppalaga to this conversation as following actions in solving issue chain depends on him (regardless of the resolution of this issue), and setting up these kind of standards may need a fresh look from direct implementers

@florianb
Copy link
Member

As reference, the Python configparser explicitly allows spaces in keys.

Our current definition of an experimental Ini grammar also allows keys containing whitespaces.

I agree, this should be more explicit, the tests should be aligned.

@xuhdev
Copy link
Member

xuhdev commented Mar 27, 2023

Do you think it's worth a vote for editorconfig/editorconfig-vote? This is a pretty corner case: Is it a decision on changing the standard, or a lack of clarity in the document?

@rosingrind
Copy link
Author

The original issue was all about clarifying this use case, however I'm in doubt if this would not require opening a vote. I'm more into opening a vote, even tho this behavior already adopted anywhere (as stated by @florianb in #39 (comment)), because embracing silently adopted features is never a good idea, especially when potential roll-back may affect a large audience. Yes, it seems like spaces in key are not prohibited, but at least some parser developers had a doubt - so right now ones are embracing it, others are not (and probably not without a reason). Assuming that you're members of editorconfig org, I'm surely leaving this up to you, but I think opening a vote may shed a light on potential problems of this use-case

@florianb
Copy link
Member

@xuhdev i didn't check how the reference implementation behaves (hadn't the time, sorry) so in the case this might break things i'd vote for a vote.

A pragmatic approach would be to align the spec and check the reference implementation: this use case is definitely aside of the use of EditorConfig and even in the wild undefined behaviour.

I also can't think of a case this might break things since there should be "only" the case a parser does not accept whitespaces which is unlikely.
Then there is only the handling of preceeding or trailing whitespaces of the keys but this is already handled by the standard.

@rosingrind
Copy link
Author

Is there any agreement? If so, feel free to close the issue with resolution status and I'll pass it back to ec4j devs

@xuhdev
Copy link
Member

xuhdev commented Jun 25, 2023

What we have in the spec right now:

  • leading whitespace before a property is ignored.
  • trailing whitespace after a property is ignored.

What we are missing: Do we allow whitespace in the middle of a property?

Based on a literal reading of the current spec:

Key: the part before the first = (trimmed of whitespace).

Whitespace within a property name isn't forbidden. In addition, since we allow non-ASCII characters, I don't see a reason that someone would have considered whitespace forbidden. Note in this case, whitespace is no different from any other characters.

In my opinion, what we should do is to clarify this point in the spec and add a test, and not consider this as a change (instead of clarification) in the standard. What do you think?

@xuhdev
Copy link
Member

xuhdev commented Jul 6, 2023

See #43 for a fix in the spec

xuhdev added a commit to editorconfig/editorconfig-core-test that referenced this issue Jul 27, 2023
xuhdev added a commit to editorconfig/editorconfig-core-test that referenced this issue Jul 27, 2023
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

No branches or pull requests

3 participants