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

Replace the WDL language grammar #6972

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

claymcleod
Copy link
Contributor

@claymcleod claymcleod commented Jul 30, 2024

This PR updates support for the WDL to the latest version (v1.2).

Description

We've written the grammar from scratch to be compliant with the latest version of WDL—currently, the grammar used in linguist is around 6 years old (and represents a very old version of WDL).

A list of some (but not all) improvements being:

  • Added all keywords (e.g., requirements).
  • Shell highlighting within command blocks.
  • Better semantic classification of different keywords.

Checklist:

(From the I am adding a new language of the template. section, as I'm adding a new example)

  • I have included a real-world usage sample for all extensions added in this PR:
    • Sample source(s):
    • Sample license(s): MIT

@claymcleod claymcleod marked this pull request as ready for review July 30, 2024 02:27
@claymcleod claymcleod requested a review from a team as a code owner July 30, 2024 02:27
@lildude lildude changed the title revise: updates the WDL language grammar Replace the WDL language grammar Jul 30, 2024
Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

Please do not change the samples. They are used to train the classifier and there will almost always be files on GitHub using older syntax.

If you want to help detect newer syntax, add more samples.

@claymcleod
Copy link
Contributor Author

Okay, I've reverted those samples and added a new one! Let me know what you think, @lildude.

@H200a

This comment was marked as spam.

@H200a

This comment was marked as spam.

@claymcleod claymcleod requested a review from lildude July 30, 2024 19:16
Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

Please also replace your sample with a real world example and update the template to state where it comes from and the license of the sample.

@claymcleod claymcleod force-pushed the revise/update-wdl branch 2 times, most recently from 29adda4 to a247e63 Compare July 30, 2024 20:37
@claymcleod
Copy link
Contributor Author

Please also replace your sample with a real world example and update the template to state where it comes from and the license of the sample.

This should be addressed: I added a new, real-world example and also added the license to the template for the PR and the header of that sample.

@claymcleod claymcleod requested a review from lildude July 30, 2024 20:43
@lildude
Copy link
Member

lildude commented Jul 31, 2024

The test failure suggests you might not have used the documented method of replacing the grammar as the script sorts the submodules.

Please use the script as documented. You can replace the grammar with itself using the script.

@claymcleod
Copy link
Contributor Author

Okay, I think I've resolved this now—I had used the script, but had done some spelunking on the codebase beforehand to try to understand how it worked (in unstaged changes).

I checked out a fresh version of the repo, applied my changes, and then got bundle exec rake running with no errors.

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

Note: this PR will not be merged until close to when the next release is made. See here for more details.

@lildude lildude added this pull request to the merge queue Aug 29, 2024
Merged via the queue into github-linguist:master with commit 8bdd2cd Aug 29, 2024
5 checks passed
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Dec 20, 2024
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.

3 participants