Skip to content

service:list in CLI#1358

Merged
mayakoneval merged 7 commits intomasterfrom
mkoneval/AP-512/service-list-cli
Jun 27, 2019
Merged

service:list in CLI#1358
mayakoneval merged 7 commits intomasterfrom
mkoneval/AP-512/service-list-cli

Conversation

@mayakoneval
Copy link
Copy Markdown
Contributor

@mayakoneval mayakoneval commented Jun 19, 2019

This PR adds service:list to the Apollo CLI tool, printing out the lists in a federated services like so:
image

Open questions:

  • is copy / pasting the functions uncaptureApplicationOutput and captureApplicationOutput necessary / how do you share those testing helpers with the shared console log? (resolved - see comment below)

TODO:

  • Update CHANGELOG.md* with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.

@apollo-cla
Copy link
Copy Markdown

@mayakoneval: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

*
* Call `uncaptureApplicationOutput` to reverse the effects of this function.
*/
function captureApplicationOutput() {
Copy link
Copy Markdown
Contributor Author

@mayakoneval mayakoneval Jun 19, 2019

Choose a reason for hiding this comment

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

this and the following function are copied from check.test.ts I am not sure how to share them with a shared console.log. They currently mock the console log output, save it and check it against the snapshot. I think these functions should be shared helper methods between all CLI tests, however when sharing them, since jest runs the tests in parallel (between files), the mocking gets overwritten. Would appreciate ideas!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmmm... I think this would actually hugely benefit from passing in a custom logger to the entirety of the apollo CLI. Probably a feature we should log somewhere, if it's not already. That way you can just instantiate the tests with a writeToMap logger that stores the log output in an array under the test name or something.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@zionts, agree on both fronts - definitely a feature we should consider adding. In the meantime, @mayakoneval can you add comments in both places that this code is duplicated by necessity? Enough to provide some context for future implementors, as they may have to do the same thing.

@mayakoneval mayakoneval requested review from evans, justinanastos and trevor-scheer and removed request for evans and trevor-scheer June 19, 2019 22:57
Comment thread packages/apollo/src/commands/service/list.ts Outdated
Copy link
Copy Markdown
Contributor

@zionts zionts left a comment

Choose a reason for hiding this comment

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

Generally looks good! Liking the added tests and the format of the output feels like just the right amount of detail to me. This is just a fly-by review since you said you were blocked. Please ping me if you want me to take another look :)

Comment thread packages/apollo-language-server/src/engine/index.ts
Comment thread packages/apollo-language-server/src/engine/index.ts
Comment thread packages/apollo/src/commands/service/list.ts
Comment thread packages/apollo/src/commands/service/list.ts Outdated
Comment thread packages/apollo/src/commands/service/list.ts Outdated
Comment thread packages/apollo/src/commands/service/list.ts Outdated
Comment thread packages/apollo/src/commands/service/list.ts
Comment thread packages/apollo/src/commands/service/list.ts Outdated
Comment thread packages/apollo/src/commands/service/list.ts
config
};

Object.assign(ctx, newContext);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thought people preferred the spread operator thing now, but idk things

@mayakoneval mayakoneval force-pushed the mkoneval/AP-512/service-list-cli branch from 3aa3ce6 to 451e931 Compare June 26, 2019 17:07
*
* Call `uncaptureApplicationOutput` to reverse the effects of this function.
*/
function captureApplicationOutput() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@zionts, agree on both fronts - definitely a feature we should consider adding. In the meantime, @mayakoneval can you add comments in both places that this code is duplicated by necessity? Enough to provide some context for future implementors, as they may have to do the same thing.

Comment thread packages/apollo/src/commands/service/__tests__/list.test.ts Outdated
Comment thread packages/apollo/src/commands/service/__tests__/list.test.ts Outdated
Comment thread packages/apollo/src/commands/service/list.ts
Comment thread packages/apollo/src/commands/service/list.ts Outdated
Comment thread packages/apollo/src/commands/service/list.ts Outdated
@mayakoneval mayakoneval force-pushed the mkoneval/AP-512/service-list-cli branch from aa5ba99 to eaad228 Compare June 27, 2019 21:26
@mayakoneval mayakoneval merged commit 07b6041 into master Jun 27, 2019
@mayakoneval mayakoneval deleted the mkoneval/AP-512/service-list-cli branch June 27, 2019 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants