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

collapse vendor folder in diffs #3234

Closed
nathany opened this Issue Sep 20, 2016 · 23 comments

Comments

Projects
None yet
4 participants
@nathany

nathany commented Sep 20, 2016

The vendor/ folder is used by Go 1.6+ and other tools/languages for third-party code. While it may be good to review all the code being used in a production app, the GitHub UI still doesn't handle large amounts of code that well.

Sorry, we could not display the entire diff because it was too big.

(It's possible to pick commits or files, but the initial load is still an issue)

Happy to open a pull request modifying generated.rb to hide vendor/** in diffs.
ref: #1549

@pchaigno

This comment has been minimized.

Show comment
Hide comment
@pchaigno

pchaigno Sep 21, 2016

Collaborator

@arfon Shoudn't github.com hide all vendored files in pull requests by default? That sounds like an issue for upstream...

Collaborator

pchaigno commented Sep 21, 2016

@arfon Shoudn't github.com hide all vendored files in pull requests by default? That sounds like an issue for upstream...

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Sep 21, 2016

I don't know -- only that linguist is currently responsible for collapsing the Godeps/ folder -- https://github.com/github/linguist/blob/master/lib/linguist/generated.rb#L314.

Something I had suggested two years ago, before Go standardized around /vendor/.
Ref: tools/godep#121

nathany commented Sep 21, 2016

I don't know -- only that linguist is currently responsible for collapsing the Godeps/ folder -- https://github.com/github/linguist/blob/master/lib/linguist/generated.rb#L314.

Something I had suggested two years ago, before Go standardized around /vendor/.
Ref: tools/godep#121

@nathany nathany changed the title from hide vendor folder in diffs to collapse vendor folder in diffs Sep 21, 2016

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Sep 28, 2016

I'd be happy to send a pull request to update generated.rb if desired.

nathany commented Sep 28, 2016

I'd be happy to send a pull request to update generated.rb if desired.

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Oct 4, 2016

Contributor

According to this line we consider this vendored which means it's excluded from the language statistics but I htink we need to update generated.rb as @nathany suggests to have them excluded from diffs.

I'd be happy to send a pull request to update generated.rb if desired.

@nathany that would be great thanks!

Contributor

arfon commented Oct 4, 2016

According to this line we consider this vendored which means it's excluded from the language statistics but I htink we need to update generated.rb as @nathany suggests to have them excluded from diffs.

I'd be happy to send a pull request to update generated.rb if desired.

@nathany that would be great thanks!

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Oct 4, 2016

Will do.

I'm only concerned with the vendor/ folder as used by Go tools. Would you like third-party, extern, etc. collapsed in diffs as well?

nathany commented Oct 4, 2016

Will do.

I'm only concerned with the vendor/ folder as used by Go tools. Would you like third-party, extern, etc. collapsed in diffs as well?

nathany added a commit to nathany/linguist that referenced this issue Oct 4, 2016

@pchaigno

This comment has been minimized.

Show comment
Hide comment
@pchaigno

pchaigno Oct 4, 2016

Collaborator

@arfon Does this mean we should treat every vendored files as generated? Wouldn't that be easier to handle upstream?

Collaborator

pchaigno commented Oct 4, 2016

@arfon Does this mean we should treat every vendored files as generated? Wouldn't that be easier to handle upstream?

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Oct 4, 2016

Contributor

Wouldn't that be easier to handle upstream?

How do you mean sorry?

Contributor

arfon commented Oct 4, 2016

Wouldn't that be easier to handle upstream?

How do you mean sorry?

@pchaigno

This comment has been minimized.

Show comment
Hide comment
@pchaigno

pchaigno Oct 4, 2016

Collaborator

Sorry that was unclear. I guess there is some closed source code that checks whether a file is generated before choosing to collapse it in diffs. Maybe this same code could also check for vendored files...?

Collaborator

pchaigno commented Oct 4, 2016

Sorry that was unclear. I guess there is some closed source code that checks whether a file is generated before choosing to collapse it in diffs. Maybe this same code could also check for vendored files...?

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Oct 4, 2016

@pchaigno Obviously I'm not familiar with any closed source you may prefer to change instead. I'm just basing this on the previous change for Godeps (#1549).

The pull request is up at #3262.
Please let me know you would like any changes.

nathany commented Oct 4, 2016

@pchaigno Obviously I'm not familiar with any closed source you may prefer to change instead. I'm just basing this on the previous change for Godeps (#1549).

The pull request is up at #3262.
Please let me know you would like any changes.

nathany added a commit to nathany/linguist that referenced this issue Oct 4, 2016

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Oct 4, 2016

Contributor

Sorry that was unclear. I guess there is some closed source code that checks whether a file is generated before choosing to collapse it in diffs. Maybe this same code could also check for vendored files...?

Ahh, gotcha. So we currently mixin BlobHelper into the view class that renders the diff on GitHub so we literally do something like:

<% unless diff_blob.generated? %>
  RENDER DIFF CONTENTS HERE
<% end %>

We could certainly expand that to include vendored? too:

<% unless diff_blob.generated? || diff_blob.vendored? %>
  RENDER DIFF CONTENTS HERE
<% end %>

which is what I think you're suggesting @pchaigno?

Contributor

arfon commented Oct 4, 2016

Sorry that was unclear. I guess there is some closed source code that checks whether a file is generated before choosing to collapse it in diffs. Maybe this same code could also check for vendored files...?

Ahh, gotcha. So we currently mixin BlobHelper into the view class that renders the diff on GitHub so we literally do something like:

<% unless diff_blob.generated? %>
  RENDER DIFF CONTENTS HERE
<% end %>

We could certainly expand that to include vendored? too:

<% unless diff_blob.generated? || diff_blob.vendored? %>
  RENDER DIFF CONTENTS HERE
<% end %>

which is what I think you're suggesting @pchaigno?

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Oct 4, 2016

Is the suggestion to make a clear distinction between vendored and generated? For example, node_modules is vendored code, not generated (https://github.com/github/linguist/blob/master/lib/linguist/generated.rb#L58).

Would the distinction be useful in the UI? I kind've feel like that is a larger refactoring that should be a separate change from this.

Personally, I consider #3262 a temporary fix. Taking a look at vendor/ code and generated code is considered a good practice by some. For now, I think we'll continue to vendor code in a separate commit so that it can be expanded and reviewed if desired, separately from the newly written code that uses it.

This is really to address the sluggish UI when GitHub diffs are too big -- or partial diffs when it knows the diff is too big, but not necessarily what to exclude. For anyone using the vendor/ folder, the first click on "Files changed" should come up faster once #3262 is in production.

nathany commented Oct 4, 2016

Is the suggestion to make a clear distinction between vendored and generated? For example, node_modules is vendored code, not generated (https://github.com/github/linguist/blob/master/lib/linguist/generated.rb#L58).

Would the distinction be useful in the UI? I kind've feel like that is a larger refactoring that should be a separate change from this.

Personally, I consider #3262 a temporary fix. Taking a look at vendor/ code and generated code is considered a good practice by some. For now, I think we'll continue to vendor code in a separate commit so that it can be expanded and reviewed if desired, separately from the newly written code that uses it.

This is really to address the sluggish UI when GitHub diffs are too big -- or partial diffs when it knows the diff is too big, but not necessarily what to exclude. For anyone using the vendor/ folder, the first click on "Files changed" should come up faster once #3262 is in production.

@pchaigno

This comment has been minimized.

Show comment
Hide comment
@pchaigno

pchaigno Oct 5, 2016

Collaborator

@arfon Yes, that's it!

Is the suggestion to make a clear distinction between vendored and generated? For example, node_modules is vendored code, not generated (https://github.com/github/linguist/blob/master/lib/linguist/generated.rb#L58).

Exactly! Same for vendors/.
I'm 👍 for #3262 being a temporary fix, but I think a proper long term fix would be make the change @arfon wrote for the view class.

Collaborator

pchaigno commented Oct 5, 2016

@arfon Yes, that's it!

Is the suggestion to make a clear distinction between vendored and generated? For example, node_modules is vendored code, not generated (https://github.com/github/linguist/blob/master/lib/linguist/generated.rb#L58).

Exactly! Same for vendors/.
I'm 👍 for #3262 being a temporary fix, but I think a proper long term fix would be make the change @arfon wrote for the view class.

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Oct 6, 2016

Contributor

I'm 👍 for #3262 being a temporary fix, but I think a proper long term fix would be make the change @arfon wrote for the view class.

I've got an open PR on github/github to rework the view class in this way. Assuming I can convince my colleagues that suppressing vendored? files in views by default is a good idea then this PR will become unnecessary.

Contributor

arfon commented Oct 6, 2016

I'm 👍 for #3262 being a temporary fix, but I think a proper long term fix would be make the change @arfon wrote for the view class.

I've got an open PR on github/github to rework the view class in this way. Assuming I can convince my colleagues that suppressing vendored? files in views by default is a good idea then this PR will become unnecessary.

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Oct 6, 2016

Thanks Arfon.

To make the UI more responsive, it would be nice to collapse vendored? initially when clicking "Files changed" (full diff).

Personally, I think it would be nice to expand vendored/generated code when looking at an individual commit that would otherwise be entirely collapsed -- or have an easy way to expand all. Then if the vendored code is in a separate commit, it can still be reviewed easily, but without slowing down the UI by including all vendored code with the full diff.

nathany commented Oct 6, 2016

Thanks Arfon.

To make the UI more responsive, it would be nice to collapse vendored? initially when clicking "Files changed" (full diff).

Personally, I think it would be nice to expand vendored/generated code when looking at an individual commit that would otherwise be entirely collapsed -- or have an easy way to expand all. Then if the vendored code is in a separate commit, it can still be reviewed easily, but without slowing down the UI by including all vendored code with the full diff.

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Oct 11, 2016

@arfon Any updates on this? It's incredibly painful to review pull requests with 100s of vendor files.

nathany commented Oct 11, 2016

@arfon Any updates on this? It's incredibly painful to review pull requests with 100s of vendor files.

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Oct 24, 2016

Just a reminder that it's been 20 days since I submitted a pull request to resolve this issue, even if only temporarily. #3262

nathany commented Oct 24, 2016

Just a reminder that it's been 20 days since I submitted a pull request to resolve this issue, even if only temporarily. #3262

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Oct 24, 2016

Contributor

Just a reminder that it's been 20 days since I submitted a pull request to resolve this issue, even if only temporarily. #3262

Sorry for the very slow response here @nathany. I've recently tested out the more 'complete' fix described above and am waiting on a 👍 to ship this by one of the engineering teams.

If I don't make progress on that today I'll move forward with #3262

Contributor

arfon commented Oct 24, 2016

Just a reminder that it's been 20 days since I submitted a pull request to resolve this issue, even if only temporarily. #3262

Sorry for the very slow response here @nathany. I've recently tested out the more 'complete' fix described above and am waiting on a 👍 to ship this by one of the engineering teams.

If I don't make progress on that today I'll move forward with #3262

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Oct 25, 2016

Contributor

@nathany - this should be fixed now. Can you confirm? Here's an example PR: chain/chain@a0e9934

screen shot 2016-10-25 at 7 07 59 am

Contributor

arfon commented Oct 25, 2016

@nathany - this should be fixed now. Can you confirm? Here's an example PR: chain/chain@a0e9934

screen shot 2016-10-25 at 7 07 59 am

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Oct 25, 2016

Thanks @arfon.

Looking at a past pull request, the vendored files are collapsed and I think it loads faster, but I was hoping it would solve another issue. Sadly it doesn't appear to.

Sorry, we could not display the entire diff because it was too big.

gocql

If it's always vendor/ code at the bottom, then that's not too much of a problem, but I think there is still room for improvement.

This is a step in the right direction though. Thanks very much for making it happen!

nathany commented Oct 25, 2016

Thanks @arfon.

Looking at a past pull request, the vendored files are collapsed and I think it loads faster, but I was hoping it would solve another issue. Sadly it doesn't appear to.

Sorry, we could not display the entire diff because it was too big.

gocql

If it's always vendor/ code at the bottom, then that's not too much of a problem, but I think there is still room for improvement.

This is a step in the right direction though. Thanks very much for making it happen!

@arfon

This comment has been minimized.

Show comment
Hide comment
@arfon

arfon Oct 25, 2016

Contributor

If it's always vendor/ code at the bottom, then that's not too much of a problem, but I think there is still room for improvement.

Understood.

/ cc @brianmario who has been thinking about how to improve some of the diff views on GitHub recently.

@nathany - are we good to close this issue now?

Contributor

arfon commented Oct 25, 2016

If it's always vendor/ code at the bottom, then that's not too much of a problem, but I think there is still room for improvement.

Understood.

/ cc @brianmario who has been thinking about how to improve some of the diff views on GitHub recently.

@nathany - are we good to close this issue now?

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Oct 25, 2016

Yup. I trust you are tracking the larger issue with Git diffs internally.

I don't see any way to improve that from linguist itself.

Cheers.

nathany commented Oct 25, 2016

Yup. I trust you are tracking the larger issue with Git diffs internally.

I don't see any way to improve that from linguist itself.

Cheers.

@nathany nathany closed this Oct 25, 2016

thaJeztah added a commit to thaJeztah/docker that referenced this issue Jan 13, 2017

don't skip 'vendor/' when indexing on GitHub
GitHub by default hides vendored code when viewing diffs, excludes them
from being searched, and navigated ("t" keyboard shortcut).

We want to be able to search, and review changes in vendored packages, so
this overrides the default, and marks vendored packages as "not vendored"
For reference, see github/linguist#1549,
github/linguist#3234

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah added a commit to thaJeztah/docker that referenced this issue Jan 13, 2017

don't skip 'vendor/' when indexing on GitHub
GitHub by default hides vendored code when viewing diffs, excludes them
from being searched, and navigated ("t" keyboard shortcut).

We want to be able to search, and review changes in vendored packages, so
this overrides the default, and marks vendored packages as "not vendored"
For reference, see github/linguist#1549,
github/linguist#3234

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@z4y4ts

This comment has been minimized.

Show comment
Hide comment
@z4y4ts

z4y4ts Mar 24, 2017

@nathany @arfon sorry for disturbing, but can you please help me with the similar issue for JS/CSS vendored files.

As far as I can see they're not being suppressed in the diffs. By a chance would you know if there plans/PR/issue/workaround to add similar feature for languages other thatn Go?

z4y4ts commented Mar 24, 2017

@nathany @arfon sorry for disturbing, but can you please help me with the similar issue for JS/CSS vendored files.

As far as I can see they're not being suppressed in the diffs. By a chance would you know if there plans/PR/issue/workaround to add similar feature for languages other thatn Go?

@nathany

This comment has been minimized.

Show comment
Hide comment
@nathany

nathany Apr 6, 2017

@z4y4ts I'm not sure, but I suggest opening a new issue rather than commenting on a closed one. You can always reference #3234 in a new issue.

Fyi, this article describes the improvements made: https://githubengineering.com/how-we-made-diff-pages-3x-faster/

nathany commented Apr 6, 2017

@z4y4ts I'm not sure, but I suggest opening a new issue rather than commenting on a closed one. You can always reference #3234 in a new issue.

Fyi, this article describes the improvements made: https://githubengineering.com/how-we-made-diff-pages-3x-faster/

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