Remove the description of inline comments at all #31
Conversation
It looks like the current wording still leads to too much confusion: #29 (comment) I therefore propose to remove the explaining addition about inline comments at all, in favor of a description of the former behavior of EditorConfig parsers.
|
Dear @xuhdev, it seems like the issue is just not solved yet so i would say we remove the inline comments entirely for clarification. Is this something we should propose a dedicated vote for (i don't think so)? |
|
I don't know if this requires a version bump. It looks like but in my opinion the change "just" makes undefined behavior to defined behavior. And elements like escaping a) weren't defined and b) were handled differently by the implemented INI-parsers. |
index.rst
Outdated
| The EditorConfig file format formerly allowed the use of `;` and `#` after the | ||
| beginning of the line to mark the rest of a line as comment. This led to | ||
| confusion how to parse values containing those characters. Old EditorConfig | ||
| parsers may still allow inline comments. |
There was a problem hiding this comment.
The leading whitespace would probably mess up the formatting...
Technically we didn't formerly "allowed" inline comment, but we have left them undefined. So the best wording might be "formerly left the use of ; and # after the beginning of the line to mark the rest of a line as comment undefined"?
|
Sorry for jumping in late! Based on the current tests, it looks like the situation is a bit of a mess :( . I think the current text should stay (i.e., I recommend not merging this PR). Per editorconfig/editorconfig-vote#6, values that look like inline comments are expressly undefined. I think it would be better for people to know it's undefined than to incorrectly assume that, because whole-line comments work, inline comments also work. |
|
No worries @cxw42 - i wish i would see a way to act immediately (and see how that time could be used in a productive way to bring EC forward). The whole attempt is pinning down a parsing issue in the dotnet roslyn compiler: dotnet/roslyn#44596 (comment) And while i agreed to the voting in 2019 i changed my mind in general. I think we should move forward and make the init file format and EditorConfig explicit. This said i'd love to see a spec going forward creating the demand for all implementers to align to the spec eventually. This wouldn't require immediate action but for everyone having to implement EC or fixing bugs it would be possible to do it in a explicit and defined way. |
|
I also tend to agree now we better explicit state that inline comments are not supported. Often people simply assume their implementation's choice of the "undefined" behavior. This issue may entrench as time goes by (maybe already too late). |
|
I agree with explicitly removing support for inline comments. Would that require a vote? As far as I can tell, editorconfig/editorconfig-vote#6 still controls. Some possible text:
|
|
I don't think so since "backslash/semicolon/octothorpe escaping" would still be "undefined" (as its not mentioned at all). I am fine with that proposal - i am not sure if it satifies the requirements of the Roslyn team. But i feel like it's something where i have to build some knowledge about writing technical specifications in english. 😬 |
|
I agree with the proposed text. Actually, not defining escaping would become natural because it's no longer needed. |
Thanks :) . Do we need a formal vote to modify editorconfig/editorconfig-vote#6 ? |
|
Maybe we can avoid modifying editorconfig/editorconfig-vote#6 by appending the text:
|
|
Revised proposed text:
Edit I asked in the roslyn issue if the above would meet their needs. |
|
Awesome @cxw42 - thank you so much! |
|
Sounds good to me! |
Are you sure this is true? I thought this remains undefined. The reason is that some implementation has already seen it as comment and we don't have the need to force them to change for now. |
|
@xuhdev I am proposing that we define it now in the specification, since it's currently implementation-defined. The tests used to include semicolons in the value - https://github.com/editorconfig/editorconfig-core-test/blob/70840cfaf6a06766ab61e975b8a1fe3b891ee08e/parser/comments.in#L14-L16 . I think Roslyn's use case shows that we need to standardize, or else lose cross-core compatibility of values that include # or ; . Please let me know if I misunderstood your question! Separately, the proposed language got some upvotes and no downvotes in the Roslyn issue. |
|
Sorry I lost the context over time. Yes, your proposed text makes sense to me. Thanks for organizing them into well-written text! |
|
Do you wanna make another PR for the proposed text? |
|
Let's just change the attached source of this PR - should i do this? |
|
Absolutely, I have no problem with that |
|
@florianb fine with me - thanks! |
|
👍 thanks a lot, i am on vacation for two weeks. I'll add it after that. If someone wants to take this over to speed it up - please feel free. |
- Clarify that inline comments are _not_ supported. See <#31 (comment)> - Bump the specification to 0.15.0 as a warning, since this change will break cores that have been supporting inline comments.
|
@florianb @xuhdev I have taken the liberty of updating this PR to:
I added new commits rather than removing the existing ones, for ease of comparison. The new commits are 645214f and 69ac8ff. Let me know what you think! If this change is approved, we will need to add back in the core tests we previously removed. That is why I thought it would be good to bump the minor version, as a sign that cores may need to change. |
- Clarify that inline comments are _not_ supported. See <#31 (comment)> - Bump the specification to 0.15.0 as a warning, since this change will break cores that have been supporting inline comments.
cxw42
force-pushed
the
florianb-patch-1
branch
from
69ac8ff to
fe197cd
Compare
|
@xuhdev would you please merge? I am not sure whether you want to squash. Thanks! |
This reverts commit 4896ac5. editorconfig/specification#31 (comment)
- Hash in value is part of the value, not a comment - Backslashes before hashes or semicolons are part of the value as well, not an escaping mechanism. editorconfig/specification#31 (comment)
- Hash in value is part of the value, not a comment - Backslashes before hashes or semicolons are part of the value as well, not an escaping mechanism. editorconfig/specification#31 (comment)
|
I have WIP editorconfig-core-test updates at https://github.com/cxw42/editorconfig-core-c/tree/no-inline-comments |
- Hash in value is part of the value, not a comment - Backslashes before hashes or semicolons are part of the value as well, not an escaping mechanism. editorconfig/specification#31 (comment)
- Hash in value is part of the value, not a comment - Backslashes before hashes or semicolons are part of the value as well, not an escaping mechanism. - This supersedes the change made in editorconfig#33. editorconfig/specification#31 (comment)
|
@xuhdev Thanks! I'll let roslyn know. I will be opening PRs shortly for the core-test and editorconfig-core-c changes :) . |
|
And @florianb --- thanks again for opening this PR and for your flexibility! 👍 |
This implements the specification changes made by <editorconfig/specification#31>. - A hash or semicolon in a value is part of the value, not the beginning of a comment. - Backslashes before hashes or semicolons are part of the value as well, not an escaping mechanism. - This supersedes the change made in editorconfig#33.
|
I have to thank you @cxw42 for pushing this forward during my absence! You are absolutely welcome! |
|
This implements the specification changes made by <editorconfig/specification#31>. - A hash or semicolon in a value is part of the value, not the beginning of a comment. - Backslashes before hashes or semicolons are part of the value as well, not an escaping mechanism. - This supersedes the change made in #33.
The EditorConfig specification now explicitly states that comments can only start at the beginning of a line and that "inline comments" are no longer recognized as such. See also: * editorconfig/specification#31 * https://github.com/editorconfig/specification/pull/31/files#diff-54a294a5d016e1a8e98bc95668ed84a99a9edd5c10394d9a2b1ee848006e98a7R91
It looks like the current wording still leads to too much confusion: #29 (comment)
I therefore propose to remove the explaining addition about inline comments at all, in favor of a description of the former behavior of EditorConfig parsers.