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 ShaderLab language #3490

Merged
merged 4 commits into from
Apr 20, 2017
Merged

Add ShaderLab language #3490

merged 4 commits into from
Apr 20, 2017

Conversation

tgjones
Copy link
Contributor

@tgjones tgjones commented Feb 22, 2017

This PR adds a new language definition for ShaderLab (used to write shaders in the Unity game engine). It uses the grammar that was already added as part of #3469.

I've moved the .shader extension from the GLSL language definition (where it was incorrect, but better than nothing) to the new ShaderLab language definition.

ShaderLab search result on GitHub.com

@larsbrinkhoff
Copy link
Contributor

.shader is a very generic file extension. I'm not a shader language expert; are really all those search result ShaderLab files?

@larsbrinkhoff
Copy link
Contributor

larsbrinkhoff commented Feb 23, 2017

I believe these keywords are present in ShaderLab but unlikely to appear in other shader languages: ColorMask, CGINCLUDE, CGPROGRAM, _MainTex, SubShader. You're right, the vast majority of all .shader files is ShaderLab.

However, some of them are GLSL.

@tgjones
Copy link
Contributor Author

tgjones commented Feb 23, 2017

Ah, thanks for noticing that. Presumably I should add the .shader extension back to GLSL - but do I need to do anything, beyond providing samples, to help the classifier?

On a possibly related note, I think there's a bug in this classifier test:

Samples.each do |sample|
  language  = Linguist::Language.find_by_name(sample[:language])
  languages = Language.find_by_filename(sample[:path]).map(&:name)
  next unless languages.length > 1

  results = Classifier.classify(Samples.cache, File.read(sample[:path]), languages)
  assert_equal language.name, results.first[0], "#{sample[:path]}\n#{results.inspect}"
end

It never gets beyond the next unless..., because Language.find_by_filename is too specific, and only ever returns a single language. Language.find_by_extension seems to work better there, but I don't know the code well enough to know if that's the correct fix.

@pchaigno
Copy link
Contributor

It never gets beyond the next unless..., because Language.find_by_filename is too specific, and only ever returns a single language. Language.find_by_extension seems to work better there, but I don't know the code well enough to know if that's the correct fix.

Nice catch! That is the correct fix :-)
I missed this test when I changed find_by_filename in #2006 :/
Could you open a separate pull request to fix it?

@larsbrinkhoff
Copy link
Contributor

do I need to do anything, beyond providing samples, to help the classifier?

If the samples doesn't do the trick, lib/linguist/heuristics.rb is your friend.

@tgjones
Copy link
Contributor Author

tgjones commented Feb 23, 2017

Could you open a separate pull request to fix it?

I've opened #3494 with a fix for the ambiguous classification test.

If the samples doesn't do the trick, lib/linguist/heuristics.rb is your friend.

It looks like the samples are enough - assuming I fixed the test correctly in #3494, and that I'm testing the right thing :)

@tgjones
Copy link
Contributor Author

tgjones commented Mar 14, 2017

Please let me know if you need anything more from me on this PR.

@larsbrinkhoff
Copy link
Contributor

Travis says the pull request doesn't pass build/test.

Note that these are copies of existing GLSL samples, renamed to have
the .shader extension.
@tgjones
Copy link
Contributor Author

tgjones commented Apr 9, 2017

Oops, somehow didn't notice that (or rather I did, but I looked at the build failure for a different Ruby version that had unrelated errors, and thought it wasn't caused by my changes).

Anyway, fixed now.

Copy link
Contributor

@larsbrinkhoff larsbrinkhoff left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@lildude lildude merged commit dd53fa1 into github-linguist:master Apr 20, 2017
Alhadis added a commit to file-icons/atom that referenced this pull request Apr 21, 2017
Added:
* .pyi to Python extensions:       github-linguist/linguist#3548
* .shader to 3D object extensions: github-linguist/linguist#3490
* .mdwn to Markdown extensions:    github-linguist/linguist#3534
* Jest icon to .js.snap files:     github-linguist/linguist#3579
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants