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

Add missing terminals to Other and ArgumentNameKeyword (#813) #814

Conversation

bathos
Copy link
Contributor

@bathos bathos commented Oct 7, 2019

See #813.


Preview | Diff

@bathos bathos changed the title add missing terminals to Other and ArgumentNameKeyword (#813) Add missing terminals to Other and ArgumentNameKeyword (#813) Oct 7, 2019
@Ms2ger
Copy link
Member

Ms2ger commented Oct 8, 2019

@bzbarsky ?

I wonder if we could lint for this.

@bathos
Copy link
Contributor Author

bathos commented Oct 8, 2019

You mentioned on #815:

At the very least, we should import that code into this repo. I wouldn't mind seeing it rewritten in python either, but that's not urgent.

I’d be happy to have a go at both integrating the postprocessor code and adding a test to check-grammar* that confirms that Other describes the set of all terminals save the seven intended exceptions. Even if they don’t share code directly, they would benefit from a bit more uniformity since they are doing very similar things. However I’d like to do so as part of a more general effort to clean the build and test scripts and, in particular, to make them platform agnostic.

Aside from the deploy script, the platform assumptions currently made seem to be avoidable. I’m on windows, so I already had to write a js equiv of the makefile to jump the initial barrier to participation, and I could polish/share that.

It would also be nice to surface this stuff more clearly. I don’t think it’s really evident that tests exist, nor is it obvious how to run locally.

I’m afraid though that if I do this, stuff will either remain JS or, in the case of bash stuff, become JS 😱. My Python is pretty limited.

* I now see this script repeats the "float" issue from #815, which might hint that there are additional worthwhile grammar checks that are missing.

Copy link
Collaborator

@bzbarsky bzbarsky left a comment

Choose a reason for hiding this comment

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

That looks reasonable, thank you!

We could try linting for this, but that would mean having the lint find all the terminals somehow.

@bzbarsky bzbarsky merged commit 4203404 into whatwg:master Oct 15, 2019
@bathos
Copy link
Contributor Author

bathos commented Oct 15, 2019

@bzbarsky

The check-grammar.js script uses syntax-cli’s Grammar class —

https://github.com/heycam/webidl/blob/4203404f78d72d448d7d32c563733a35fa99b0ac/check-grammar.js#L40

— which exposes methods like getTerminals()

I haven’t dug in enough to say for sure, but it appears that the syntax-cli module provides the grammar introspection tools which would be needed to perform the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants