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

Update Go (golang) handling #3800

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@AlekSi

AlekSi commented Aug 29, 2017

Closes #3719.
Refs #2724.

/cc @lildude @cespare

@AlekSi

This comment has been minimized.

Show comment
Hide comment
@AlekSi

AlekSi Aug 29, 2017

I'm conflicted about the second point. Vendored files are not generated, they are vendored, that is clear. But why all vendored files are not suppressed in diffs?

AlekSi commented Aug 29, 2017

I'm conflicted about the second point. Vendored files are not generated, they are vendored, that is clear. But why all vendored files are not suppressed in diffs?

@lildude

A few inline questions.

# Returns true or false.
def godeps?
!!name.match(/Godeps\//)
end

This comment has been minimized.

@lildude

lildude Oct 8, 2017

Member

'scuse my ignorance, but why is this being removed? Is it because all Godeps will always match Code generated .* DO NOT EDIT implemented above?

@lildude

lildude Oct 8, 2017

Member

'scuse my ignorance, but why is this being removed? Is it because all Godeps will always match Code generated .* DO NOT EDIT implemented above?

This comment has been minimized.

@AlekSi

AlekSi Oct 8, 2017

Because vendored files are not generated files. Vendoring is already covered by https://github.com/github/linguist/blob/master/lib/linguist/vendor.yml#L51

@AlekSi

AlekSi Oct 8, 2017

Because vendored files are not generated files. Vendoring is already covered by https://github.com/github/linguist/blob/master/lib/linguist/vendor.yml#L51

# Go vendored dependencies
refute sample_blob("vendor/vendor.json").generated?
assert sample_blob("vendor/github.com/kr/s3/sign.go").generated?
refute fixture_blob("go/food_vendor/candy.go").generated?

This comment has been minimized.

@lildude

lildude Oct 8, 2017

Member

Wouldn't these tests still "pass" with the changes above?

@lildude

lildude Oct 8, 2017

Member

Wouldn't these tests still "pass" with the changes above?

generated_sample_without_loading_data("go/vendor/github.com/foo.go")
generated_sample_without_loading_data("go/vendor/golang.org/src/foo.c")
generated_sample_without_loading_data("go/vendor/gopkg.in/some/nested/path/foo.go")

This comment has been minimized.

@lildude

lildude Oct 8, 2017

Member

Same here, wouldn't these tests still "pass" with the changes above?

@lildude

lildude Oct 8, 2017

Member

Same here, wouldn't these tests still "pass" with the changes above?

# Godep saved dependencies
generated_sample_without_loading_data("Godeps/Godeps.json")
generated_sample_without_loading_data("Godeps/_workspace/src/github.com/kr/s3/sign.go")

This comment has been minimized.

@lildude

lildude Oct 8, 2017

Member

And here too?

@lildude

lildude Oct 8, 2017

Member

And here too?

@lildude

This comment has been minimized.

Show comment
Hide comment
@lildude

lildude Dec 2, 2017

Member

Nudge

Member

lildude commented Dec 2, 2017

Nudge

@lildude

This comment has been minimized.

Show comment
Hide comment
@lildude

lildude Mar 28, 2018

Member

No response in three months. Marking as stale.

Member

lildude commented Mar 28, 2018

No response in three months. Marking as stale.

@lildude lildude added the Stale label Mar 28, 2018

@AlekSi

This comment has been minimized.

Show comment
Hide comment
@AlekSi

AlekSi Mar 28, 2018

Vendored files are not generated, they are vendored, that is clear. But why all vendored files are not suppressed in diffs?

That is the important question, and answering may answer your questions. It looks like different parts of linguist treat "vendored" and "generated" words differently.

AlekSi commented Mar 28, 2018

Vendored files are not generated, they are vendored, that is clear. But why all vendored files are not suppressed in diffs?

That is the important question, and answering may answer your questions. It looks like different parts of linguist treat "vendored" and "generated" words differently.

@lildude

This comment has been minimized.

Show comment
Hide comment
@lildude

lildude Apr 6, 2018

Member

But why all vendored files are not suppressed in diffs?

Hmmm, vendored files generally aren't suppressed in diffs. Only generated should be. That said, very large diffs are minimised in diffs, regardless of whether they're vendored or not. Do you have examples of where you're seeing discrepancies that I can see and try explain?

Member

lildude commented Apr 6, 2018

But why all vendored files are not suppressed in diffs?

Hmmm, vendored files generally aren't suppressed in diffs. Only generated should be. That said, very large diffs are minimised in diffs, regardless of whether they're vendored or not. Do you have examples of where you're seeing discrepancies that I can see and try explain?

@pchaigno

This comment has been minimized.

Show comment
Hide comment
@pchaigno

pchaigno Apr 26, 2018

Collaborator

@AlekSi Friendly nudge.

Collaborator

pchaigno commented Apr 26, 2018

@AlekSi Friendly nudge.

@AlekSi

This comment has been minimized.

Show comment
Hide comment
@AlekSi

AlekSi Jun 9, 2018

Hmmm, vendored files generally aren't suppressed in diffs. Only generated should be.

PTAL: percona/pmm-managed@6e3e7d2 Here vendored files are not rendered, but the message says "Some generated files are not rendered by default." But those files are vendored, not generated.

AlekSi commented Jun 9, 2018

Hmmm, vendored files generally aren't suppressed in diffs. Only generated should be.

PTAL: percona/pmm-managed@6e3e7d2 Here vendored files are not rendered, but the message says "Some generated files are not rendered by default." But those files are vendored, not generated.

@pchaigno

This comment has been minimized.

Show comment
Hide comment
@pchaigno

pchaigno Jun 13, 2018

Collaborator

There's some background on this in #2843 and #2620.

As far as I understand, we mark these files as generated---even though they're not really---precisely because we want to hide them by default in diffs.

Collaborator

pchaigno commented Jun 13, 2018

There's some background on this in #2843 and #2620.

As far as I understand, we mark these files as generated---even though they're not really---precisely because we want to hide them by default in diffs.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 7, 2018

I'd like to see linguist updated to use the https://golang.org/s/generatedcode specification, and it seems this PR is working in that direction.

From the discussion, it seems that to make progress, the PR should be updated to leave out vendor files, since they're out of scope. Is my understanding right?

dmitshur commented Oct 7, 2018

I'd like to see linguist updated to use the https://golang.org/s/generatedcode specification, and it seems this PR is working in that direction.

From the discussion, it seems that to make progress, the PR should be updated to leave out vendor files, since they're out of scope. Is my understanding right?

@lildude

This comment has been minimized.

Show comment
Hide comment
@lildude

lildude Oct 16, 2018

Member

Nudging this for one last time.

Member

lildude commented Oct 16, 2018

Nudging this for one last time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment