refactor: move things around #241
Conversation
googlebot
added
the
cla: yes
There was a problem hiding this comment.
Only some small concerns are commented, but overall it looks great, and it's really helpful to make the code more readable and cleaner, thanks @alexander-fenster !
| npm install | ||
| npm test | ||
| npm run fix | ||
| npm run compile | ||
| npm run system-test | ||
| npm run docs | ||
| npm run docs-test |
There was a problem hiding this comment.
Is this intentional to remove docs-test?
There was a problem hiding this comment.
Yes. I think it does not make sense to check if links in the generated docs are valid because if they are not, it's more likely not a generator failure. We disabled this check for DLP and might also want to disable it for monitoring, which probably means it's better to disable it for all libraries.
| @@ -3,7 +3,7 @@ google/protobuf/descriptor.proto | |||
| google/api/annotations.proto | |||
| google/protobuf/duration.proto | |||
| google/protobuf/timestamp.proto | |||
| google/kms/v1/resources.proto | |||
| google/cloud/kms/v1/resources.proto | |||
There was a problem hiding this comment.
I'm a little bit confused here, if we generated google/kms/v1/resources.proto before instead of /google/cloud/kms.... in proto_list, that sounds like a bug?
There was a problem hiding this comment.
Originally kms lived in protos/google/kms, I just moved it to protos/google/cloud/kms to match the googleapis location.
| @@ -7,8 +7,5 @@ | |||
| }, | |||
| "include": [ | |||
| "typescript/**/*.ts" | |||
| ], | |||
| "exclude":[ | |||
| "typescript/test/test_application_ts/src/*.ts" | |||
There was a problem hiding this comment.
i'm not sure the reason why we excluded the file in the tsconfig here, if it's safe to remove it?
There was a problem hiding this comment.
Because it's now outside of typescript folder (it's a fixture, so it's in fixtures).
| @@ -0,0 +1,137 @@ | |||
| // Copyright 2020 Google LLC | |||
There was a problem hiding this comment.
This looks great, much cleaner to have a separate file for code map!
| describe('Baseline tests', () => { | ||
| initBaselineTest(); | ||
|
|
||
| runBaselineTest({ |
There was a problem hiding this comment.
This template is so cool, thanks for doing this!
alexander-fenster
force-pushed
the
tests
branch
from
d797935 to
0a62164
Compare
alexander-fenster
force-pushed
the
tests
branch
from
0a62164 to
13eb629
Compare
In this PR I'm moving a lot of things around to achieve the following goals:
circleci.ymltypescript/folder contains only*.tsfiles, fixtures and protos moved out