Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Comments

Another large rewrite#62

Merged
winstliu merged 13 commits intomasterfrom
wl-another-large-rewrite
Jan 10, 2017
Merged

Another large rewrite#62
winstliu merged 13 commits intomasterfrom
wl-another-large-rewrite

Conversation

@winstliu
Copy link
Contributor

@winstliu winstliu commented Jan 9, 2017

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

For some reason whenever I work on language-yaml I end up rewriting the whole thing each time. Oh well. This PR should probably have been split up into like 10 different ones, but many of the changes depend on the others.

  • Pulled standalone - stuff (aka tags without a colon) into its own match
    • This greatly simplifies the main tag match, meaning that we no longer have three nested non-capturing groups
  • Ensure that spaces follow the ending colon (fixes the second part of Incorrect syntax highlights #53)
  • Use a consistent blacklist across tag matches (fixes the first part of Incorrect syntax highlights #53)
    • Note: somehow I messed up the issue number twice in the commit. The one listed here is correct.
  • Fix constant.other.date.yaml being misapplied to the entire match, rather than just the date
  • Yank date and numeric matching into a repository so that they can be included when necessary, rather than making custom matches for them that would only work under certain scenarios
  • Integrate !!omap matching into the existing tag matches
  • Make variable matching stricter
  • Moved many rules (comments, strings, variables) into the repository to reduce code duplication and maintenance
  • Properly match non-specific tag indicators (key: ! value)
  • Don't match leading whitespace as an unquoted string
  • Properly match merge-key tags (<<: *variable-to-merge)
  • Remove old rules that were never used

Alternate Designs

None. This is an overall rewrite to the grammar to hopefully make it easier to maintain in the future. Code duplication was getting out of hand, and many of the more obscure rules weren't working properly.

Benefits

Better overall code tokenization and accuracy.

Possible Drawbacks

Possible regressions, as is possible with any rewrite. Spec coverage has been improving each rewrite though, so the chance of severe regressions happening shouldn't be that large anymore.

Applicable Issues

Fixes #53

* Pulled standalone `- stuff` (aka tags without a colon) into its own
match
* This greatly simplifies the main tag match, meaning that we no longer
have three nested non-capturing groups
* Ensure that spaces follow the ending colon (fixes the second part of
#52)
* Use a consistent blacklist across tag matches (fixes the first part of
#51)
* Fix `constant.other.date.yaml` being misapplied to the entire match,
rather than just the date
* Integrate `!!omap` matching into the existing tag matches
* Make variable matching stricter
Also don't match leading whitespace as unquoted strings
'name': 'comment.line.number-sign.yaml'
'2':
'name': 'punctuation.definition.comment.yaml'
'include': '#comment'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MaximSokolov: do you think this scope name is ok, or would switching it to a keyword like keyword.other.tag.non-specific.yaml be better?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know. Seems to me it's more than just a punctuation mark. According to documentation (3.3.2. Resolved Tags), there is also a ? tag, does it have a scope?

@winstliu
Copy link
Contributor Author

Continuing the conversation here so that it's more visible: no, it currently doesn't. The ! tag was already there, so I kept it, but didn't add any of the other ones. That said, they look similar, so I could just group them together: [?!](?=\\s)

@MaximSokolov
Copy link
Contributor

I am concerned about using punctuation.definition.keyword.yaml scope name for !!, because syntax--keyword selector can override styles for syntax--punctuation. It should be changed either to punctuation.definition.smth-else.yaml or keyword.other.smth.yaml.

As for ! tag, I'll leave final decision to you, as I am not quite familiar with YAML syntax

@winstliu
Copy link
Contributor Author

punctuation.definition.tag.omap?

@MaximSokolov
Copy link
Contributor

punctuation.definition.tag.omap

👍

@winstliu
Copy link
Contributor Author

I think I'm going to avoid adding more scopes in this PR. Those can be added separately.

@winstliu winstliu merged commit 9e57ab9 into master Jan 10, 2017
@winstliu winstliu deleted the wl-another-large-rewrite branch January 10, 2017 23:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect syntax highlights

2 participants