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

Conversation

kevinastone
Copy link
Contributor

@kevinastone kevinastone commented Apr 22, 2018

Description of the Change

Introduce syntax tests to improve the test coverage. Anticipating changes for the tree-sitter migration (cc @maxbrunsfeld + @ambv), we'd benefit from having a more complete set of grammar tests to ensure compatibility. This begins developing those test fixtures.

This introduces a few syntax fixture files and fixes a bug with missing vararg and kwarg syntax highlighting.

Benefits

atom-grammar-test is a test helper to allow you to provide code snippets and examples that you annotate with the desired syntax rules. It makes it much easier to validate the grammar rules and allows parties to provide examples of missing of errant syntax highlighting.

Possible Drawbacks

Dependency on another package? It's been used by several atom language grammars to date.

Applicable Issues

N/A

@ambv
Copy link
Contributor

ambv commented Apr 22, 2018

This is very cool. I'll leave merging to @50Wliu or @maxbrunsfeld but +1 from me.

I'd like more comprehensive grammar fixtures though. I can generate you some more code for this later.

@kevinastone
Copy link
Contributor Author

@ambv, yep! I wanted to see if this would land before committing to more fixtures. Just working on this exposed quite a few gaps in the grammar (type annotations, punctuation, etc)

grammarTest path.join(__dirname, 'fixtures/grammar/syntax_test_python_lambdas.py')
grammarTest path.join(__dirname, 'fixtures/grammar/syntax_test_python_typing.py')

xdescribe "SQL highlighting", ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disabled this section since it was failing for me. Does this work for others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore this. It runs fine and I removed it.

@kevinastone kevinastone force-pushed the syntax-test branch 2 times, most recently from 65de2b6 to 2764738 Compare April 24, 2018 00:39
@kevinastone
Copy link
Contributor Author

Also improved the Type declarations and added more syntax tests.

@winstliu
Copy link
Contributor

Also improved the Type declarations

Could this be moved into a separate PR?

@kevinastone
Copy link
Contributor Author

Sure, will move to another PR: https://github.com/kevinastone/language-python/tree/typing-improvements
Removed the commit.

@kevinastone
Copy link
Contributor Author

Ready

package.json Outdated
"devDependencies": {
"coffeelint": "^1.10.1"
"coffeelint": "^1.10.1",
"atom-grammar-test": "^0.6.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this needs to be in the dependencies section, or else tests won't run on atom/atom (atom/language-html@624139d). I don't think the situation has changed since that commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

weird. ok. fixed.

Copy link
Contributor

@winstliu winstliu left a comment

Choose a reason for hiding this comment

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

This looks ok to me. I'll let it sit for a while longer to see if @maxbrunsfeld has comments.

@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld commented May 1, 2018

@50Wliu I'm pretty unfamiliar with the textmate grammars, so I'm good with this as long as it looks good to you.

@kevinastone just to check - have you done manual testing of your grammar changes to verify that everything looks good visually?

@kevinastone
Copy link
Contributor Author

@maxbrunsfeld yeah, it renders better due to support for asterisks in args and kwargs.

Before

screenshot 2018-05-01 21 31 00

After

screenshot 2018-05-01 21 30 41

@kevinastone
Copy link
Contributor Author

Bare lambda expressions are also fixed:

Before

screenshot 2018-05-01 21 33 51

After

screenshot 2018-05-01 21 34 11

@winstliu winstliu merged commit 253d032 into atom:master May 3, 2018
@kevinastone kevinastone deleted the syntax-test branch May 3, 2018 00:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants