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 KoLmafia ASH with samples and heuristic #6784

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gausie
Copy link

@gausie gausie commented Apr 4, 2024

Description

I am currently missing a grammar - the closest thing that exists is here https://raw.githubusercontent.com/danielyxie/vscode-kolmafia-ash/master/syntaxes/ash.yml. How would you recommend I include that?

Checklist:

@gausie gausie requested a review from a team as a code owner April 4, 2024 10:20
Typo in visit_url
@lildude
Copy link
Member

lildude commented Apr 4, 2024

There's already a PR for this - #5401. Maybe you can contribute to that PR or pull in the changes proposed there, keeping in mind all the comments on that PR.

@gausie
Copy link
Author

gausie commented Apr 4, 2024

There's already a PR for this - #5401. Maybe you can contribute to that PR or pull in the changes proposed there, keeping in mind all the comments on that PR.

Oh wow well done for remembering this. And sorry @midgleyc for missing your work! I think the search I've done, which is just a regex based on the heuristic I wrote, does a better job of reflecting the popularity.

I'll update this PR to use Chris's grammar.

@gausie
Copy link
Author

gausie commented Apr 4, 2024

Ok that is done. I note that @midgleyc's PR seems to have run script/list-grammars but the guide for adding a new language does not include that as a step - would you like me to do so?

@midgleyc
Copy link

midgleyc commented Apr 4, 2024

This is likely to fail the popularity test again -- I think the number of required repos has only increased since my PR several years ago.

A lot of repositories exist only on Sourceforge, and as GitHub doesn't (seem to?) list repos without recent contributions in the search, finding many of the existing scripts is going to be difficult. Even with them, the language likely isn't popular enough.

@gausie
Copy link
Author

gausie commented Apr 4, 2024

This is likely to fail the popularity test again -- I think the number of required repos has only increased since my PR several years ago.

A lot of repositories exist only on Sourceforge, and as GitHub doesn't (seem to?) list repos without recent contributions in the search, finding many of the existing scripts is going to be difficult. Even with them, the language likely isn't popular enough.

The search I have in the description is listing almost 1k files, not sure how to see individual repos but it does seem to be at least a hundred.

It would be really great to differentiate these from AGS Scripts!

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.

We can't accept your samples for a few reasons:

  1. they're too big; if the diff suppresses them, they're too big for our needs
  2. they're not covered by an open source license.

All samples must be covered by an open source license and this must be stated in the template and identifiable at the time in the source repo. We will not accept samples without an indication of their license.

We're also going to need the cached license file. This should have been created when you ran script/add-grammar to add the grammar. Please add that file to the PR. If you can't find it, run this command:

linguist/script/add-grammar

Lines 133 to 134 in 4ac734c

log 'Caching grammar license'
bundle exec licensed cache -c vendor/licenses/config.yml

As with the other PR, popularity still isn't sufficient for inclusion right now.

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

Successfully merging this pull request may close these issues.

None yet

4 participants