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

Directive syntax is not documented in language grammar #949

Open
nguerrera opened this issue Aug 30, 2022 · 14 comments
Open

Directive syntax is not documented in language grammar #949

nguerrera opened this issue Aug 30, 2022 · 14 comments
Milestone

Comments

@nguerrera
Copy link
Contributor

I noticed that we don't have the directive syntax documented in the grammar.

I further noticed that directives invalidate this comment since the trailing newline is significant to how a directive is parsed.

https://github.com/microsoft/cadl/blob/eb955d2fd79a3857760b1aba8a9bd6b9ea41c54c/packages/spec/src/spec.emu.html#L195-L200

@ghost ghost added the Needs Triage label Aug 30, 2022
@bterlson
Copy link
Member

Interesting... I recall we discussed the #suppression syntax was supposed to mimic decorators and given the linebreak isn't necessary (AFAIK) maybe we just drop that part of it? So like #suppress "blah" "blah" model foo { } becomes valid-but-ugly?

@bterlson
Copy link
Member

Ahh I take it back, I see you can have multiple suppression arguments. Yeah, this syntax is strange and I have regrets.

@timotheeguerin
Copy link
Member

we could change it(require parentheses)

is that section not also invalid for single line comments?

@nguerrera

This comment was marked as outdated.

@nguerrera
Copy link
Contributor Author

nguerrera commented Aug 30, 2022

Actually, you're right, we do need to specify what ends a single line comment. I'd be fine with saying newlines are CR, CRLF, or LF, period.

@timotheeguerin
Copy link
Member

I do think though that not being able to run #suppress inline is an issue too

@nguerrera
Copy link
Contributor Author

Hmm, we have LineTerminator referenced there, but not defined. Shouldn't grammarkdown have gotten mad?

@nguerrera
Copy link
Contributor Author

If we're willing to take a breaking change, it should probably have been #directive(arg1, ..., argN). But that's a big break now, I think.

@timotheeguerin
Copy link
Member

string literals also have some concept of new lines

image

@bterlson
Copy link
Member

Ecmarkup is possibly using the ecmascript biblio, where the LineTerminator is defined. I think recent versions should not use it by default though.

I would also be fine just saying line breaks are what we currently support, but I know LS and PS are also common choices here.

@nguerrera
Copy link
Contributor Author

nguerrera commented Aug 30, 2022

I think LS and PS aren't worth supporting. Then you have to wonder "why not NEL?":

https://github.com/microsoft/TypeScript/blob/main/src/compiler/scanner.ts#L484-L485

Swift only supports CR, LF, and CRLF as well, and I don't think anyone will actually want to use anything else in practice in source code.

@nguerrera
Copy link
Contributor Author

I think we can use this issue to update the grammar to reflect the current reality including fixing that incorrect comment about newline significance and having a definition of LineTerminatator that matches our current implementation.

And we can file separate issues if we want to change directive syntax or support additional newline characters. I'm inclined to say no to both: too breaking, and not worth it, so I will let someone else who might feel strongly about either of those file issues.

@allenjzhang allenjzhang added design:needed A design request has been raised that needs a proposal and removed Needs Triage labels Sep 2, 2022
@nguerrera
Copy link
Contributor Author

I don't think we need design here. We can update the spec to match the reality. If we want changes, then separate design issues can be filed.

@markcowl markcowl removed the design:needed A design request has been raised that needs a proposal label Sep 13, 2022
@markcowl markcowl added this to the [2022] November milestone Sep 13, 2022
@markcowl
Copy link
Contributor

pri: 1
estimate: 3

@markcowl markcowl added this to the [2023] May milestone Jan 28, 2023
@markcowl markcowl modified the milestones: [2023] May, Backlog Apr 5, 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

6 participants