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

chore(deps): upgrade to gts v2 #232

Merged
merged 11 commits into from
Mar 19, 2020
Merged

chore(deps): upgrade to gts v2 #232

merged 11 commits into from
Mar 19, 2020

Conversation

JustinBeckwith
Copy link
Contributor

@JustinBeckwith JustinBeckwith commented Feb 3, 2020

This is using a pre-release version of gts, but I think it's ready for a look. I'd honestly be ok with merging it as is. I want to put a few more miles on the new system before releasing 2.0.0.

A few things to discuss:

  • I used a more minimal prettier config that adds the spaces back around brackets, and leaves out trailing commas. I don't care about either of these, so I prefer whatever prettier tells me is best.
  • We need to be way more careful with prettierignore and eslintignore with the new gts. We can no longer rely on the tsconfig to tell us what files to look at

Feedback welcomed 🤗

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

bcoe commented Feb 3, 2020

I can't wait to merge this 👍

@JustinBeckwith JustinBeckwith changed the title DO NOT MERGE: migrate to gts2 chore(deps): upgrade to gts v2 Feb 4, 2020
@@ -13,14 +13,14 @@
// limitations under the License.

// [START gaxios_quickstart]
const {request} = require('gaxios');
const { request } = require('gaxios');
Copy link
Contributor

Choose a reason for hiding this comment

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

So... how about disabling this option in prettier config to make the result much closer to what we have now?

        "bracketSpacing": {
          "description": "Print spaces between brackets.",
          "default": true,
          "type": "boolean"
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nods we absolutely could do that. I was thinking of reducing the amount of prettier customizations we do, and keep it as close to the default config as possible. That includes dropping bracket spacing, and the trailing commas.

If we want to optimize for lowest changes, we could keep adding those customizations in.

Copy link
Contributor

@alexander-fenster alexander-fenster Feb 4, 2020

Choose a reason for hiding this comment

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

Oh. There is a serious risk of starting a discussion about style here :)

I get your point about keeping minimal config, but one thing about gts default style is that it more or less did what's explained in Google JavaScript Style Guide. For some reason I feel it might make sense to adhere to that style guide :) It puts no space between brackets, and uses trailing commas.

image

image

But again, I just thought that keeping the formatting (if not too hard) is a good idea, but I'm OK with either decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good :) gts update here:
google/gts#440

@codecov
Copy link

codecov bot commented Feb 4, 2020

Codecov Report

Merging #232 into master will increase coverage by 0.65%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
+ Coverage   97.84%   98.49%   +0.65%     
==========================================
  Files           6        4       -2     
  Lines         602      599       -3     
  Branches      102       95       -7     
==========================================
+ Hits          589      590       +1     
+ Misses         13        9       -4     
Impacted Files Coverage Δ
src/common.ts 100.00% <100.00%> (ø)
src/gaxios.ts 96.56% <100.00%> (-0.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e62967...c17bdb0. Read the comment docs.

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

This is looking very reasonable to me, I appreciate that the code changes are fairly minimal.

@bcoe bcoe merged commit d26a325 into master Mar 19, 2020
@bcoe bcoe deleted the gts2 branch March 19, 2020 17:20
yoshi-automation added a commit that referenced this pull request Apr 1, 2020
d26a325
commit d26a325
Author: Justin Beckwith <justin.beckwith@gmail.com>
Date:   Thu Mar 19 10:20:31 2020 -0700

    chore(deps): upgrade to gts v2 (#232)
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