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

Revert #4306 - Stop marking go.mod and go.sum generated #4340

Merged
merged 6 commits into from
Jan 16, 2019

Conversation

lildude
Copy link
Member

@lildude lildude commented Dec 3, 2018

As discussed in the original PR #4306, whilst the go.mod and go.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 and go.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

@pchaigno
Copy link
Contributor

pchaigno commented Dec 3, 2018

If we add them as Text, won't we have to add corresponding sample files? The sample files may skew the Bayesian classifier...

@lildude
Copy link
Member Author

lildude commented Dec 3, 2018

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.

@pchaigno
Copy link
Contributor

pchaigno commented Dec 5, 2018

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 .ncl files.

@lildude
Copy link
Member Author

lildude commented Dec 5, 2018

But we don't need to add them 😉

@pchaigno
Copy link
Contributor

pchaigno commented Dec 5, 2018

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.

@lildude
Copy link
Member Author

lildude commented Dec 8, 2018

Should we add a test to make sure they're not added?

We could do, though other tests will pick up if they're added to Go (or elsewhere) whilst still being associated with "Text".

@pchaigno
Copy link
Contributor

pchaigno commented Dec 8, 2018

I probably wasn't very clear; I'm afraid someone could add samples of these files in samples/Text/.

@borgstrom
Copy link

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 go.mod as "text" seems fine to me. It could be satisfied with a pattern in the .mod section that matches the module location/path in the file. I don't have enough context with linguist to know if that addresses @pchaigno's concern or not.

I think that classifying go.sum is a little bit harder, and would opt for having it treated the same way that Gopkg.lock (or package-lock.json, requirements.txt, etc) are treated by leaving it as a "generated" file. All of these other lock files are also "widely expected for these to properly reviewed" but still require the viewer to click "load diff", so I think there's a pretty good case for it being the same in the go mod world.

@myitcv
Copy link

myitcv commented Jan 14, 2019

Any update on this, @lildude?

@lildude
Copy link
Member Author

lildude commented Jan 14, 2019

@myitcv I plan to finish this off today or tomorrow.

@lildude
Copy link
Member Author

lildude commented Jan 15, 2019

Test will start passing again once #4383 has been merged and I've pulled master into this branch.

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.

LGTM!

@lildude lildude changed the title Revert #4306 - Correcting go.mod and go.sum to generated files instead of AMPL. Revert #4306 - Stop marking go.mod and go.sum generated Jan 16, 2019
@lildude lildude merged commit 5819331 into master Jan 16, 2019
@lildude lildude deleted the lildude/revert-4306 branch January 16, 2019 16:41
@myitcv
Copy link

myitcv commented Jan 31, 2019

Quick question - when is this likely to go live on GitHub?

@lildude
Copy link
Member Author

lildude commented Jan 31, 2019

Quick question - when is this likely to go live on GitHub?

Hopefully sometime next week, depending on my workload.

@Alhadis Alhadis mentioned this pull request Oct 3, 2019
2 tasks
@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