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

build: add config .gitattributes #647

Merged
merged 6 commits into from
Jun 26, 2020

Conversation

summer-ji-eng
Copy link
Contributor

@summer-ji-eng summer-ji-eng commented Jun 24, 2020

add .gitattributes into node template to fix windows linter fail on end of line break issue #464

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 24, 2020
Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two asks for this PR :)

  1. I would like us to flag the protos directory broadly as "generated" code, so linguist stops counting most of our repos as JavaScript. Turns out, you modify .gitattributes to do that! Figured 92 PRs are better than 184.
  2. I am scared of getting infinity [CHANGE ME] PRs. @bcoe should we do a hammer PR first, or does that break the autosynth tooling? I think for individual commits to work like this, our autosynth backlog has to be super clean.

@bcoe
Copy link
Contributor

bcoe commented Jun 24, 2020

[CHANGE ME] PRs. @bcoe should we do a hammer PR first

This should no longer be needed with @SurferJeffAtGoogle's work, changes to synthtool will show up with whatever commit message we use carried over.

I would just change this to:

build: add config .gitattributes perhaps

@summer-ji-eng summer-ji-eng changed the title fix: windows lint build: add config .gitattributes Jun 24, 2020
@summer-ji-eng
Copy link
Contributor Author

Try to understand I would like us to flag the protos directory broadly as "generated" code, so linguist stops counting most of our repos as JavaScript. Turns out, you modify .gitattributes to do that! Figured 92 PRs are better than 184.
Sorry, I guess I missed some context here. Why make modify .gitattributes relate to protos directory. And figured 92 PRs refer to which PR, any links? @JustinBeckwith could you give me more details?

@JustinBeckwith
Copy link
Contributor

JustinBeckwith commented Jun 24, 2020

Heh, so feel free to ignore this for now. The context: GitHub shows you the language for a given repository like this:
image
https://github.com/googleapis?q=&type=&language=javascript

It will also show you the language breakdown in the repo page:
image
https://github.com/googleapis/nodejs-firestore

GitHub uses linguist to perform this calculation. In our case - half our repos show as TypeScript, the other half JavaScript, mostly because of the amount of JavaScript we generate in that protos directory. I would like GitHub to surface these as TypeScript, both for vanity and for tooling purposes.

Using the .gitattributes file, you can tell linguist to ignore particular glob patterns, or even remap them! I was asking if we could do that work in this PR, so we don't need to do this, then fire another 90 PRs when we add these exclude rules. We probably should test to make sure it works with a single repository first. I am ok with skipping it for now.

@summer-ji-eng
Copy link
Contributor Author

@JustinBeckwith thanks for context. It is really helpful.
As I learned from https://github.com/github/linguist#using-gitattributes, .gitattribtues file can override linguist-documentation, linguist-language, linguist-vendored, linguist-generated and linguist-detectable attributes.

So our change should no mess up linguist.
Moreover, maybe we could try add
protos/*.js linguist-language=Typescript in .gitattributes to tell linguist the client lib repo is typescript project.

Before I merge this PR, maybe we should use one nodejs client library to test. Does nodejs-firestore a good one to test or do you have any other suggestion. LMK. Thanks

@JustinBeckwith
Copy link
Contributor

You got it! Firestore tends to be a little weird compared to our other repositories. I should have picked a better example. Maybe https://github.com/googleapis/nodejs-scheduler/?

@summer-ji-eng
Copy link
Contributor Author

Cool. Thanks. Will try out on nodejs-scheduler

@JustinBeckwith JustinBeckwith merged commit dc9caca into googleapis:master Jun 26, 2020
@summer-ji-eng
Copy link
Contributor Author

test on nodejs-tasks googleapis/nodejs-tasks#430
it works as expected
before add .gitattributes the languages in nodejs-tasks looks like this:
Screen Shot 2020-06-26 at 2 53 10 PM
after merge, current looks like:
Screen Shot 2020-06-26 at 3 11 34 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants