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

Fix ambiguous classification test #3494

Closed
wants to merge 1 commit into from
Closed

Fix ambiguous classification test #3494

wants to merge 1 commit into from

Conversation

tgjones
Copy link
Contributor

@tgjones tgjones commented Feb 23, 2017

Found while preparing #3490.

This change means that the test does now find some ambiguous languages, and tests that the relevant samples resolve to the correct language, but it fails with this assertion:

  1) Failure:
TestClassifier#test_classify_ambiguous_languages [/Users/timjones/Code/github/linguist/test/test_classifier.rb:53]:
/Users/timjones/Code/github/linguist/samples/Makefile/filenames/Makefile.frag
[["JavaScript", -3411.2346496412188], ["GLSL", -3498.798416469136]].
Expected: "Makefile"
  Actual: "JavaScript"

Presumably this is because there are some filename overrides, including for Makefile, that aren't picked up if we use Language.find_by_extension. Any suggestions for fixing this?

@tgjones tgjones mentioned this pull request Feb 23, 2017
@tgjones
Copy link
Contributor Author

tgjones commented Apr 9, 2017

Bump - any thoughts on my question above?

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.

Presumably this is because there are some filename overrides, including for Makefile, that aren't picked up if we use Language.find_by_extension. Any suggestions for fixing this?

Sorry about the delay here. Yup, this is it. Makefile.frag is an explicitly called out filename for Makefiles so shouldn't need classification by extension, hence the failure.

We need to do both filename and extension as shown in #3709 (which reminded me that I'd seen this before which brought me here).

I'll leave it to you and @smola to determine who is going to ultimately fix this, or you can race for it 😁

@lildude
Copy link
Member

lildude commented Jul 22, 2017

Closing as this issue has now been fixed with #3709

@lildude lildude closed this Jul 22, 2017
@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.

2 participants