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 the wording of inserting # or ; to the end of a line #29

Merged
merged 2 commits into from Jan 12, 2022

Conversation

xuhdev
Copy link
Member

@xuhdev xuhdev commented Jul 15, 2021

@florianb
Copy link
Member

@xuhdev, with all respect to @jedmao, i found the sentence a bit hard to read and interprete, would you mind if i propose a slightly simpler variant:

Inserting a unescaped # or ; after non-spaces on a line (i.e. inline) is not parsed as a comment, nor as part of the section name, the key pair (see below), or the value it was inserted into. This behavior may change in the future; therefore this type of insertion is not recommended.

If you are fine with that i wouilkd commit this and we merge it today?

@xuhdev
Copy link
Member Author

xuhdev commented Jan 12, 2022

@florianb The first "a" should be "an", otherwise it looks good to me.

@florianb
Copy link
Member

@xuhdev thank you very much – done.

@florianb florianb merged commit a6baaa5 into master Jan 12, 2022
@florianb
Copy link
Member

@xuhdev i was so free to merge it – I hope this relieves you a little and I get used to doing a little more here.

Comment on lines +76 to +80
- Inserting an unescaped ``#`` or ``;`` after non-whitespace characters in
a line (i.e. inline) is not parsed as a comment, nor as part of
the section name, the key pair (see below), or the value it was inserted
into. This behavior may change in the future; therefore this kind of
insertion is not recommended.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This text is still not clear to me. For the specific case where ; is included in a property value:

property_name = aaaa;bbbb

Which of the following behaviors is expected?

  1. The property value is aaaa (; and everything after it are ignored)
  2. The property value is aaaa;bbbb (; has no special meaning)
  3. The property value is aaaabbbb (; has no special meaning for comments, but is not parsed as part of the value)

According to literal interpretation of this text, (3) appears to be the only allowed answer; however, I do not think this is the behavior that was intended.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - ; has no special meaning, if it's not at the beginning of the line.

I guess i will rework the specification to remove negations. I think this whole paragraph is a leftover from former interpretations and implementations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The word "unescaped" leaves the impression there's a way for escaping. But escaping is not allowed even though some ini-parsers may provide given mechanisms. I am sorry for the confusion.

Copy link
Member Author

@xuhdev xuhdev Jan 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would be easier to just say something like "what # or ; after non-whitespace characters do is undefined". What do you think about the following:

Inserting a # or ;, escaped or not, after non-whitespace characters in a line (i.e. inline) is not guaranteed to be parsed as a comment, nor as part of the section name, the key pair (see below), or the value it was inserted into. Therefore this kind of insertion leads to undefined behavior and is not recommended. However, this may change in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the specification should not leave any way for interpretation, so a conforming core is not allowed to parse it as comment. And that's the reason why (i should come to this conclusion earlyier) i would remove that paragraph or add it as "informative" anecdote.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the editorconfig and ini-parsing in general are two different things because ini-parsing is heavily context specific where ini-parsing isn't. I open a PR to o draft my idea..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks this is still not clear. But we need clarification for fixing this anyoning "bug" dotnet/roslyn#44596

@xuhdev xuhdev deleted the end-comment branch January 12, 2022 19:35
florianb added a commit that referenced this pull request May 20, 2022
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.
xuhdev pushed a commit that referenced this pull request Oct 30, 2022
* Remove the description of inline comments at all

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.

* Simplify the reference to formerly allowed inline comments

* Make a new subsection for the definitions of terms

* No inline comments; bump spec to 0.15.0

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

Co-authored-by: Chris White <cxwembedded@gmail.com>
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 this pull request may close these issues.

None yet

5 participants