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

A couple of things #1

Closed
t-mart opened this issue Sep 21, 2022 · 5 comments · Fixed by #2
Closed

A couple of things #1

t-mart opened this issue Sep 21, 2022 · 5 comments · Fixed by #2

Comments

@t-mart
Copy link

t-mart commented Sep 21, 2022

Hi there. I've been doing some testing on TOC syntax and found, what I believe to be, a few issues with this grammar.

  • All official tags can be picked up by the client case-insensitively, except for localized tags, where just the locale part is case-sensitive.

    # case insensitive ok, but rendered as invalid
    ## TITLE: Foo
    
    # ok because locale matches sensitively, but rendered as invalid
    ## TITLE-enUS: Bar
    
    # custom/invalid, but weird splitting on hyphen
    ## Title-ENUS: Bar
  • The locales enCN, ptPT, enTW are missing. I do not have access to test these locales, but they do seem to be listed in places where other locales are, such as in the source of interface/sharedxml/videooptionspanels.lua or in some filenames.

    # rendered as invalid, but should be official
    ## Title-enCN: Foo
    ## Title-ptPT: Foo
    ## Title-enTW: Foo
  • The Dep[^:]+ pattern should be Dep[^:]* for 0 or more characters instead of 1 or more (if I understand this regex flavor correctly). In other words, simply Dep should be acceptable.

    # rendered as invalid, but should be official
    ## Dep: Foo
    • Additionally, Dependencies does not need to be listed in the pattern with the previous pattern.
  • A space in a tag name/key is syntactically disallowed -- the line will not be read as a tag at all by the client. Instead, functionally, the whole line becomes a comment at that point, so maybe we should render it as such. (Note: Other whitespace like tabs are okay in keys, but the client will render them with a little □).

    # rendered as a tag, but should be comment
    ## Spaced Key: is invalid
  • Spacing around tag keys and values is arbitrary, and can even be non-spaced.

    # rendered as comment, but should be official tag
    ##Title:Foo
    
    # already works correctly
    ##      Title    :     Foo    
  • Tags require colons, or else they won't be accessible by the client (like above). Again, should probably be a comment.

    # rendered as tag, but should be comment
    ## Foo
  • I'm not sure Description is ever used by official WoW code, so its inclusion as an official/known tag might not be correct.

    # rendered as official/known tag, but maybe should be custom/invalid
    ## Description: Foo
  • Note, when I say "invalid" tag name/key above, I'm using the same language you used at https://github.com/nebularg/language-toc-wow/blob/master/grammars/toc-wow.cson#L23.

    However, I feel like this wording should be revisited: It's not really "invalid" as much as it is "non-conforming" or "unofficial" or "user-defined" or something like that. It doesn't break parsing to use these names, and they're still accessible in the client.

    Speaking of user-defined tags, it is conventional to prefix user-defined tags with X-, but that doesn't seem to be enforced by the client. In fact, one of the most widely-used addons has a TOC tag that lacks that prefix.

    I feel like the dichotomy should be, perhaps:

    • "official" tags that are read by the client for some use
    • "user-defined" tags (X-prefixed or otherwise) that are used by other addons or third-party programs.
@nebularg
Copy link
Owner

wow, I haven't thought about this for years and no longer use atom 😜 But apparently linguist picked it up? I suppose that merits some updates

nebularg added a commit that referenced this issue Sep 24, 2022
nebularg added a commit that referenced this issue Sep 24, 2022
@nebularg nebularg mentioned this issue Sep 24, 2022
@nebularg
Copy link
Owner

If you want to check that out. I probably broke commenting on directive lines, but i'll check to see how the game handles whitespace and comments later

@t-mart
Copy link
Author

t-mart commented Sep 26, 2022

apparently linguist picked it up

Yes, indeed. I'm surprised you didn't know about it. Perhaps someone submitted it on your behalf. EDIT: might have been in this commit: github-linguist/linguist@9ae19a1

Nonetheless, this project does have wide-reaching effects with many WoW addons being git-hosted here on GitHub, so I am glad that you are receptive to keeping it maintained.

If you want to check that out

I'll have to see how I can test this. Perhaps either by running Atom (which I don't use either) or by running linguist (I'm really out of date with Ruby).

@nebularg
Copy link
Owner

I tested it by overwriting my vscode extension grammar that a friend adapted from this because I was too lazy to publish it :p https://gist.github.com/nebularg/7a69d820d7a35480cc367969714c6d69

I did check in-game and doing something like

## X-Comment: Hello # hi

The # hi is included in the return string, no need to add an exception there.

I agree 'invalid' isn't the best name for non-standard tags, but changing it to a subscope in entity.name.tag would get it colored like the rest without custom syntax rules and I'd rather it stand out.

@t-mart
Copy link
Author

t-mart commented Sep 26, 2022

The # hi is included in the return string, no need to add an exception there.

I'm glad to hear this. My heart sank when you mentioned it because I just had submitted pygments/pygments#2244 (a Python syntax highlighter) and didn't consider this case. But, thanks for confirming that I do not need to.

I agree 'invalid' isn't the best name for non-standard tags, but changing it to a subscope in entity.name.tag would get it colored like the rest without custom syntax rules and I'd rather it stand out.

That's fine. No big deal.

I wasn't able to load the new grammar into anything, but your updates in your gist look good at addressing the issues.

It looks like linguist updates grammar submodules when they do a release, so we'll just have to wait until then. Their past release schedule isn't consistent, so 🤷‍♂️.

I'm going to close this, and hope to see your merge of #2 soon. Thanks!

@t-mart t-mart closed this as completed Sep 26, 2022
nebularg added a commit that referenced this issue Sep 29, 2022
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.

2 participants