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 Wren language #4088

Closed
wants to merge 4 commits into from
Closed

Add Wren language #4088

wants to merge 4 commits into from

Conversation

bates64
Copy link

@bates64 bates64 commented Apr 3, 2018

Added the Wren scripting language (grammar & sample). cc @munificent

Checklist:

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.

Thanks for the contribution. Along with addressing my inline comments, you'll need to address the two failing tests too.

Additionally, whilst there are a lot of .wren files, they're only distributed across 19 repos owned by 19 different people so doesn't meet the "used by hundreds of repositories" requirement yet. I've labelled the PR accordingly.

@@ -0,0 +1,15 @@
System.print("Hello, world!")

Copy link
Member

Choose a reason for hiding this comment

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

Linguist needs real-world usage examples to properly train the classifier. "Hello world" code is never real-world usage code. Please replace this with an example of a file from a real project and provide a reference to it and it's license in a comment in the PR.

Choose a reason for hiding this comment

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

type: grammar
name: wren-sublime
license: other
---
Copy link
Member

Choose a reason for hiding this comment

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

Can you please update your fork with the latest master, and try replacing the grammar with itself using:

script/add-grammar --replace wren-sublime https://github.com/munificent/wren-sublime

This should hopefully use the new Licensed code to correctly and automatically detect the license.

@bates64
Copy link
Author

bates64 commented Apr 7, 2018

Looks like licenses are still broken - could this be something to do with wren-sublime using a different header than just "MIT License"? GitHub the site doesn't seem to detect that it's MIT either.

@munificent I notice you use a similar "$project is licensed under the MIT License:" header in a lot of your repositories. Is there a specific reason for this, as it looks like it trips up GH?

@bates64
Copy link
Author

bates64 commented Apr 7, 2018

@lildude script/add-grammar is failing due to script/list-grammars throwing an error for the scope text.conllu :/

script/list-grammars:81:in `block in to_markdown': undefined method `chomp' for nil:NilClass (NoMethodError)
	from script/list-grammars:65:in `each'
	from script/list-grammars:65:in `to_markdown'
	from script/list-grammars:96:in `update_readme'
	from script/list-grammars:105:in `<main>'

Any ideas?

@lildude
Copy link
Member

lildude commented Apr 12, 2018

Hmmm, I'm not sure why you're getting that error without seeing the full backtrace.

I'll see if I can reproduce this locally on a clean installation.

@Alhadis
Copy link
Collaborator

Alhadis commented Apr 12, 2018

@lildude It's because licensee is throwing an error over a non-absolute system path. It happens in the add-grammar script and leaves Linguist with a half-registered submodule.

This fixes it:

diff --git a/script/add-grammar b/script/add-grammar
index 1d16f063..5a0b0b81 100755
--- a/script/add-grammar
+++ b/script/add-grammar
@@ -111,6 +111,7 @@ log "Confirming license"
 if repo_old
   command('script/licensed')
 else
+  repo_new = File.absolute_path(repo_new)
   command('script/licensed', '--module', repo_new)
 end

@lildude lildude mentioned this pull request Apr 12, 2018
2 tasks
@lildude
Copy link
Member

lildude commented Apr 12, 2018

It's because licensee is throwing an error over a non-absolute system path.

Ahhhh. Makes sense. I like your fix better than mine. Stealing borrowing it for my PR 😉

@bates64
Copy link
Author

bates64 commented Apr 13, 2018

Tried to apply your fix, @Alhadis, but nothing seems to have changed :/

Deregistering: vendor/grammars/wren-sublime
$ git submodule deinit vendor/grammars/wren-sublime
$ git rm -rf vendor/grammars/wren-sublime
$ script/grammar-compiler update -f
Registering new submodule: vendor/grammars/wren-sublime
$ git submodule add -f https://github.com/munificent/wren-sublime vendor/grammars/wren-sublime
$ script/grammar-compiler add vendor/grammars/wren-sublime
Confirming license
$ script/licensed
Updating grammar documentation in vendor/README.md
$ bundle exec rake samples
$ script/sort-submodules
$ script/list-grammars
  > script/list-grammars:81:in `block in to_markdown': undefined method `chomp' for nil:NilClass (NoMethodError)
  > 	from script/list-grammars:65:in `each'
  > 	from script/list-grammars:65:in `to_markdown'
  > 	from script/list-grammars:96:in `update_readme'
  > 	from script/list-grammars:105:in `<main>'
Command failed. Aborting.

I can try this on a clean branch from master if that might help?

@Alhadis
Copy link
Collaborator

Alhadis commented Apr 13, 2018

@heyitsmeuralex It's an issue with your submodules list. Git submodules are managed outside the usual versioning mechanics. Make sure you've merged the latest changes from master, and with your branch checked-out, run script/bootstrap and git submodule sync.

When you hit the error the first time, a submodule was probably registered in your local config, even if it wasn't necessarily checked out in the vendor/grammars directory.

This is I opened #4056 (which has been mostly addressed in #4097). It's a pretty nasty (and less-than-obvious) UX issue.

@munificent
Copy link

I notice you use a similar "$project is licensed under the MIT License:" header in a lot of your repositories. Is there a specific reason for this, as it looks like it trips up GH?

No specific reason. I've been copy/pasting that file for ages, since long before GitHub even existed, much less detected licenses. :) I can try to fix it to something GitHub can recognize.

@lildude
Copy link
Member

lildude commented Apr 17, 2018

No specific reason. I've been copy/pasting that file for ages, since long before GitHub even existed, much less detected licenses. :) I can try to fix it to something GitHub can recognize.

That would be ✨ for Linguist but also all the visitors to your repos that use this format for the license file as GitHub will be able to automatically detect the license and add it to the top of the repo just above the language stats bar.

@pchaigno
Copy link
Contributor

I can try to fix it to something GitHub can recognize.

Licensee (the GitHub project that detects licenses) uses https://choosealicense.com/ as its primary source for license texts. If you use that formatting for the MIT license, you can be sure it will be recognized by Licensee.

@munificent
Copy link

OK, I went and fixed the LICENSE files for wren, wrenalyzer, and wren-sublime. I'll eventually work my way through all my various random little repos, but that might take a while. :)

@pchaigno
Copy link
Contributor

pchaigno commented Aug 4, 2018

Closing in favor of #4219. We'll monitor the popularity of the extension there.

If you have an improved search query compared to the one used in #4219, please post it here and I'll update #4219.

@pchaigno pchaigno closed this Aug 4, 2018
@Nixinova Nixinova mentioned this pull request May 1, 2021
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
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.

5 participants