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 ; and # within values #23

Closed
sharwell opened this issue Mar 2, 2021 · 14 comments · Fixed by #31
Closed

Clarify behavior of ; and # within values #23

sharwell opened this issue Mar 2, 2021 · 14 comments · Fixed by #31

Comments

@sharwell
Copy link

sharwell commented Mar 2, 2021

The specification contains the following language:

Inserting a # or ; after non-whitespace characters in a line (i.e., inline) shall neither be parsed as a comment nor as part of the section name, pair (defined below) key or value in which it was inserted. This may change in the future; thus, is not recommended.

It also contains the following test inputs:

https://github.com/editorconfig/editorconfig-core-test/blob/70840cfaf6a06766ab61e975b8a1fe3b891ee08e/parser/comments.in#L14-L20

https://github.com/editorconfig/editorconfig-core-test/blob/70840cfaf6a06766ab61e975b8a1fe3b891ee08e/parser/comments.in#L35-L41

With the following tests:

https://github.com/editorconfig/editorconfig-core-test/blob/70840cfaf6a06766ab61e975b8a1fe3b891ee08e/parser/CMakeLists.txt#L83-L85

https://github.com/editorconfig/editorconfig-core-test/blob/70840cfaf6a06766ab61e975b8a1fe3b891ee08e/parser/CMakeLists.txt#L99-L101

This has led me to the following questions:

  1. What are the expected results for test6.c and test12.c?
  2. The test expectations for test5.c and test11.c appear to be in direct violation of formal specification. Which of these two is expected to be correct?
  3. For users wishing to define a .editorconfig value containing a literal ; or # character (.editorconfig file_header_template: can't use # (or ;) symbol dotnet/roslyn#44596), what is the supported mechanism?
@sharwell

This comment has been minimized.

@xuhdev xuhdev transferred this issue from editorconfig/editorconfig-core-test Mar 3, 2021
@xuhdev
Copy link
Member

xuhdev commented Mar 3, 2021

This seems like an oversight in the spec language (#12) that none of us happened to catch. @jedmao Do you recall anything regarding this?

@jednano
Copy link
Member

jednano commented Mar 3, 2021

This wasn't an oversight. I thought it was a bit of a silly rule, but preserved it because of the existing tests that were mentioned above.

@xuhdev
Copy link
Member

xuhdev commented Mar 3, 2021

@jedmao I don't think the language preserves the existing tests though. The current test says that if you have a colon after a value, it should be taken as part of the value. The language says that it shouldn't be taken as part of the value, at least for now.

@jednano
Copy link
Member

jednano commented Mar 3, 2021

Oh, right. I think that @florianb and I agreed that that particular rule wasn’t ideal, but we should probably have had the spec align with current implementations.

@florianb
Copy link
Member

florianb commented Mar 3, 2021

Yeah - i can't recall the full discussion, yet,

But we skipped support for inline comments for the revision of ini parsing and specification entirely because of it's ambiguity.

I agree we should change the spec.

@MichaelKetting
Copy link

@florianb This ticket's been in limbo since March 2021. Do you have an update about the status of spec? Not being able to use semi-colons in string values of editor.config prevents some powerful usage scenarios from moving forward.

Much appriciated, Michael

@florianb
Copy link
Member

florianb commented Jan 8, 2022

Thank you very much for pinging me @MichaelKetting – you are absolutely right. What a pity.

@editorconfig/board-member Regarding this case and the thoughts we already put into a rework/creation of a ini grammar are there any objections to try remove inline comments in general?

I think introducing a viable escape mechanism would be a second option, though I think it would be more ergonomic to try to abandon inline comments at first.

Given there are no objections i'd try to push this forward including taking on Jed's work at the spec PR #29 . Thank you all very much.

@xuhdev
Copy link
Member

xuhdev commented Jan 8, 2022

@florianb No objection from my side.

@xuhdev
Copy link
Member

xuhdev commented Jan 11, 2022

@florianb #29 is only a clarification PR -- it doesn't require any changes related to the tests. I think it's safe to approve and merge if you agree with the changes in #29 .

@florianb
Copy link
Member

Yeah thanks @xuhdev – i thought waiting some days for response may have gained more feedback. 😅

@florianb
Copy link
Member

@MichaelKetting you might have noticed we merged a change to the specification explicitly disallow inline comments in the ini-parsing for EditorConfig. The next step will be aligining the EditorConfig parsers out there.

@cxw42
Copy link
Member

cxw42 commented Jul 11, 2022

Following up --- it looks like #29 and editorconfig/editorconfig-vote#6 are contradictory on the surface. To help out dotnet/roslyn#44596 , could we define it one way or the other?

Edit Would it make sense to open an issue in editorconfig/editorconfig asking for votes about whether people expect ; and # in a value to start a comment or not?

@xuhdev
Copy link
Member

xuhdev commented Jul 18, 2022

Following up --- it looks like #29 and editorconfig/editorconfig-vote#6 are contradictory on the surface. To help out dotnet/roslyn#44596 , could we define it one way or the other?

@cxw42 The reason those two are contradicted is that the poll happened later than #29, and we decide to go the way as the poll has decided.

Edit Would it make sense to open an issue in editorconfig/editorconfig asking for votes about whether people expect ; and # in a value to start a comment or not?

That would be a useful survey, but I don't think it's worth the effort to massively change the behavior (which is not really a particularly groundbreaking feature any way)

@cxw42 cxw42 linked a pull request Oct 29, 2022 that will close this issue
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.

6 participants