Repository navigation
Allow classifier to run on symlinks as usual - #3948
Conversation
|
I've thought of this quite recently and came up with a similar idea, however my reluctance with going this route is the one question: who decides on the weighting? We might think Markdown is more important than "GCC Machine Description", however a "GCC Machine Description" dev without a single markdown file in his repo is going to argue the other way. I picture this becoming quite a battle when it comes to C/C++/Objective-C and their constant battle with misclassification of header files. I'm not completely averse to the idea of a weighting, I just want to be sure we've got a good strategy to deal with how we determine this weighting. A quick thought (without much other thought 😉 )... with regards to following the symlinks which is causing this problem, why not bail really early with no results when we encounter a symlink? Effectively completely ignoring the symlink. The destination, if in the repo, should be assessed in its own right and if the destination isn't in the repo, it doesn't count anyway. |
Yes, that's a very good point. However;
In this case the Bayesian classifier will always choose a winner for us, and we can just let that do its thing. I envision this as a way to resolve ties when there aren't other good means. In this case, we're not following symlink contents because we're accepting that the symlink is a way people use to render a given file as a different filename. The example here is: there's a
I definitely agree, and I'm not exactly sure what that would look like, and am definitely open to other methods of doing this. We could just do something similar to the "popular" list and bless certain filetypes such as Markdown. |
If we want to choose Markdown at the end, why even go through the Bayesian classifier? We could bail early at the heuristic strategy with Markdown as a default result for |
Well, for non-symlink files, we do want to consider the content, and there are some legitimate GCC Machine Description files which the classifier will correctly identify. We just want to resolve a tie in case of not considering the content, such as with symlinks. We don't go through the classifier at all with symlinks presently. |
|
As far as I understand, since the Bayesian classifier always returns the complete list of candidate languages ordered by scores (i.e., it doesn't exclude any candidate language), the following will always return the language with the highest weight, regardless of the score assigned by the classifier: ls = languages.each_with_index.sort_by { |l, idx| [-l.weight, idx] }.first
ls[0] if ls |
|
Ah, of course, you're quite right. I'll have to think of a better solution here. |
User |
|
Since the classifier runs last, I've tried having it simply return its first candidate only. This feels like a hack, but I'm not sure there's a strict downside. |
e039246 to
067b8cc
Compare
|
Sorry to leave this hanging around for so long. I've been playing around with the changes you introduced in #3946 and one thing I'm not clear on is why we're exiting early in the classifier if it's a symlink? I've done some testing with master as it stands after #3946 has been merged, and by removing this line I can get Linguist to always correctly identify the file through a symlink. For example, symlinking Another example, using the Perl/POD file from github/markup#1133 as ... and checking the original file: So as I understand it, things are operating as expected if we don't exit the classifier early if it's a symlink, however I suspect I'm missing something somewhere. |
|
I suspect I'm missing something somewhere, but your results check out, so let's try running with that. I'll cook up a PR. |
067b8cc to
0a040fa
Compare
|
PR force-pushed per your suggestion. This looks like it'll check out. 👍 |
| ["#{samples_path}/#{language_name}/filenames"] | ||
| fixs = Dir.glob("#{samples_path}/#{language_name}/#{file}") - | ||
| ["#{samples_path}/#{language_name}/filenames"] | ||
| fixs.reject { |f| File.symlink?(f) } |
There was a problem hiding this comment.
Do we need to filter out the symlinks during testing? Wouldn't doing so effectively mean we never test a symlink with the heuristics?
Additionally, I think we need a specific heuristics test for symlinks to ensure the heuristics are being skipped for them.
There was a problem hiding this comment.
The problem is the test symlink's placement in the Markdown directory means we assume it should return Markdown as the result, whereas we skip heuristics instead and return nothing. Before this change to the test, we get this output:
1) Failure:
TestHeuristics#test_md_by_heuristics [/Users/kivikakk/github/linguist/test/test_heuristics.rb:191]:
Failed for /Users/kivikakk/github/linguist/samples/Markdown/symlink.md.
--- expected
+++ actual
@@ -1 +1 @@
-[#<Linguist::Language name=Markdown>]
+[]I'll add an explicit test too.
lildude
left a comment
There was a problem hiding this comment.
Great! Thanks for the explanation about the test too.
|
I might have missed something, but why don't we want heuristic rules to run on symlinks? |
When deciding between "GCC Machine Description" and "Markdown" for
.md, and a symlink is involved, we always choose "GCC Machine Description".In general, if we've gotten to the end of all strategies and we're choosing between these two — i.e. we've deemed it to be "equal probability" that it's either one or the other — however we've determined this, we should choose Markdown as the more likely of the two.
/cc @lildude Definitely open to other ways of doing this.