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

Correct misclassification of .js.erb files #4427

Merged
merged 4 commits into from
May 31, 2019
Merged

Correct misclassification of .js.erb files #4427

merged 4 commits into from
May 31, 2019

Conversation

CHTJonas
Copy link
Contributor

Change the language classification of *.js.erb files from HTML+ERB to JavaScript+ERB. I think I've covered all the relevant bases but let me know if there's something I can correct - thanks for taking the time to review! 😄

Description

Previously JavaScript files containing embedded Ruby syntax were misclassified as HTML+ERB. This PR would correct this by classifying such source code files as JavaScript+ERB and grouping under the parent JavaScript language.

Motivation for this is that JavaScript files meant for client-side execution in response to an AJAX request in a Rails application are listed under HTML when running github-linguist --breakdown.

Checklist:

Previously JavaScript files containing embedded Ruby, for example
files used to render JavaScript for client-side execution in response
to an AJAX request in a Rails application, were misclassified as HTML+ERB.
This commit corrects this by classifying such source files as JavaScript+ERB
and grouping under the parent JavaScript language.

Signed-off-by: Charlie Jonas <charlie@charliejonas.co.uk>
@stale
Copy link

stale bot commented Mar 20, 2019

This pull request has been automatically marked as stale because it has not had recent activity, and will be closed if no further activity occurs. If this pull request was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions.

@stale stale bot added the Stale label Mar 20, 2019
@pchaigno pchaigno removed the Stale label Mar 21, 2019
Copy link
Contributor

@pchaigno pchaigno 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 pull request and sorry for the delay in reviewing!

I left two comments related to syntax highlighting below.

For other reviewers: I think there is a large enough corpus of these files on GitHub to mandate support: extension:erb filename:js.

lib/linguist/languages.yml Outdated Show resolved Hide resolved
lib/linguist/languages.yml Outdated Show resolved Hide resolved
@CHTJonas
Copy link
Contributor Author

CHTJonas commented Mar 23, 2019

Thanks for the comments @pchaigno! Syntax highlighting updated now to use JavaScript now.

I wasn't sure whether tm_scope should be source.js (same as JavaScript language proper) or whether I should add something like text.js.erb to the grammars.yml file to match what HTML+ERB and NetLinx+ERB do?

@pchaigno
Copy link
Contributor

I wasn't sure whether tm_scope should be source.js (same as JavaScript language proper) or whether I should add something like text.js.erb to the grammars.yml file to match what HTML+ERB and NetLinx+ERB do?

The grammar.yml should not be manually updated so source.js is fine. If we're using JavaScript for both the TextMate scope and the CodeMirror grammar, we might as well add the extension to the list of JavaScript extensions. It would achieve the exact same effect.

@CHTJonas
Copy link
Contributor Author

I was considering that, but I think(?) listing separately does allow some benefits eg. when searching you can restrict the scope to JavaScript+ERB as opposed to all JavaScript, such as this example with HTML+ERB.

@stale
Copy link

stale bot commented Apr 22, 2019

This pull request has been automatically marked as stale because it has not had recent activity, and will be closed if no further activity occurs. If this pull request was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions.

@stale stale bot added the Stale label Apr 22, 2019
@CHTJonas
Copy link
Contributor Author

Did you have any thoughts on the above RE: whether to keep the .js.erb extension separate or just add it to the list of JS extensions @pchaigno?

@stale stale bot removed the Stale label Apr 23, 2019
@stale
Copy link

stale bot commented May 23, 2019

This pull request has been automatically marked as stale because it has not had recent activity, and will be closed if no further activity occurs. If this pull request was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions.

@stale stale bot added the Stale label May 23, 2019
@stale stale bot removed the Stale label May 27, 2019
@lildude lildude requested review from pchaigno and Alhadis May 31, 2019 08:29
@lildude lildude merged commit 9847fcf into github-linguist:master May 31, 2019
@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.

None yet

4 participants