search

davidcelis / language-thrift

Atom syntax highlighting and snippets for Thrift files.
MIT License
1 stars 5 forks source link

List separators (, or ;) should be optional, but throw off syntax highlighting when not present #1

Open bcronin opened 10 years ago

bcronin commented 10 years ago

Screenshot of the syntax highlighting error:

image

davidcelis commented 9 years ago

Hey, sorry it's taken me so long to comment on this. The problem here is that you haven't put in the required trailing semicolons, so the syntax is invalid. Here's what it looks like with the required semicolons added:

bcronin commented 9 years ago

Preface: this issue is obviously very minor! I really don't mind it staying closed.

Semicolons are optional according to the closest-thing-to-official Thrift IDL docs. Snippet from the spec doc (namely the ? after the ListSeparator is the point of interest):

[12] Struct          ::=  'struct' Identifier 'xsd_all'? '{' Field* '}'
...
[16] Field           ::=  FieldID? FieldReq? FieldType Identifier ('= ConstValue)? XsdFieldOptions ListSeparator?
...
[40] ListSeparator   ::=  ',' | ';'

Also, the thrift compiler compiles such structs without warning or error (at least on v0.9.2 which is the version I'm working with -- which is not the latest release AFAIK).

On the other side of the argument, it's fair to point out the To Do/Questions section of the IDL docs which contains the note: "What to do about ListSeparator? Do we really want to be as lax as we currently are?"

Anyway, just wanted to point out that it seems (via the not-so-strict-spec and the compiler behavior) that semicolons are optional.

davidcelis commented 9 years ago

Ack I just realized that what I really meant were commas; shouldn't field definitions be comma-separated? The Thrift IDL docs you linked to have an example file used by their own tests, which involves commas between field and function definitions...

bcronin commented 9 years ago

Well, the IDL implies any ListSeparator is optional (if I'm interpreting the ? symbol after it correctly) -- and I do know the thrift compiler v0.9.2 accepts fields without commas or semicolons (since that's still what's in our codebase).

I certainly don't feel strongly about the issue! If I'm the first and last person to have noticed the issue, it's probably not worth your time to fix :)

davidcelis commented 9 years ago

It does look like ListSeparator is optional. So here's the deal; I'd fix this, but I'm not terribly familiar with the grammar and how to make that work in the cson file. This is foremost a simple port of the Thrift tmbundle and I haven't done much editing with it. I also haven't had to write any Thrift since I left New Relic, so I'm unfortunately not in a rush to fix that.

I would be happy to accept a PR to fix the issue of optional ListSeparators, and willing to give a commit bit to anybody who wants to do so and is more familiar with the Thrift grammar!