-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Revert #4306 - Stop marking go.mod and go.sum generated #4340
Conversation
This reverts commit ba0c5e9.
If we add them as Text, won't we have to add corresponding sample files? The sample files may skew the Bayesian classifier... |
Nope because we'd never get to the classifier as there'd only be one language associated with the filenames so Linguist will return that early. |
But the sample files for a language are used by the Bayesian classifier for all extensions and filenames of that language. So adding these Go files may skew the results for say |
But we don't need to add them 😉 |
Sure. Should we add a test to make sure they're not added? I'm worried someone might add them in the future ignoring the reason for their absence. |
We could do, though other tests will pick up if they're added to Go (or elsewhere) whilst still being associated with "Text". |
I probably wasn't very clear; I'm afraid someone could add samples of these files in |
Hi. 👋 Wanted to chime in here following #4357 (I still don't know how I missed this issue when I scanned the open PRs 🤦♂️) Using the heuristics to classify I think that classifying |
Any update on this, @lildude? |
@myitcv I plan to finish this off today or tomorrow. |
Files taken from https://github.com/golang/oauth2/ licensed under BSD 3-clause license
Test will start passing again once #4383 has been merged and I've pulled master into this branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Quick question - when is this likely to go live on GitHub? |
Hopefully sometime next week, depending on my workload. |
As discussed in the original PR #4306, whilst the
go.mod
andgo.sum
files are indeed generated, it is widely expected for these to properly reviewed within a PR so shouldn't be suppressed in the diffs, as happens for generated files.Of course this does mean we're back to them being incorrectly identified as another language and getting it's wonky syntax highlighting too. We can't associate these directly with Go as named files as they would then be treated as written in Go, which isn't the case, and as a result get wonky syntax highlighting then too.
Ideally having the files in Go syntax would be best, but failing that, the only other option I can think of it so add these files to
Text
so they don't get syntax highlighting, don't count towards the stats, and don't get suppressed in diffs either.I'm open to suggestions on how to best address this part of things.
I've added a
go.mod
andgo.sum
file to the test fixtures to ensure they continue to be classified as text going forward. These two files are taken from https://github.com/golang/oauth2/ which is licensed under BSD-3-Clause:Template removed as it's not relevant