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

CLI Context Renaming #3061

Closed
4 tasks
alexanderbez opened this issue Dec 10, 2018 · 11 comments · Fixed by #6290
Closed
4 tasks

CLI Context Renaming #3061

alexanderbez opened this issue Dec 10, 2018 · 11 comments · Fixed by #6290
Assignees
Labels

Comments

@alexanderbez
Copy link
Contributor

alexanderbez commented Dec 10, 2018

Summary

CLIContext has grown out of a refactor of the client package (rightfully so) and has become a powerhouse in facilitating much of the client functionality. However, I'm seeing it being used in REST components and other areas where a "CLI" is not really involved.

Proposal

Both the CLI and REST I see as client interfaces. I simply propose we rename CLIContext to client.Context.

/cc @jackzampolin @alessio @fedekunze


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@fedekunze
Copy link
Collaborator

++

@jackzampolin
Copy link
Member

Proposal Accepted!

@jackzampolin jackzampolin self-assigned this Dec 14, 2018
@jackzampolin jackzampolin added REST T: Dev UX UX for SDK developers (i.e. how to call our code) and removed gaia-lite labels Jan 28, 2019
@jackzampolin jackzampolin added this to the v0.38.0 milestone Sep 17, 2019
@alexanderbez alexanderbez mentioned this issue Oct 10, 2019
4 tasks
@alexanderbez alexanderbez removed this from the v0.38.0 milestone Nov 30, 2019
@fedekunze
Copy link
Collaborator

I think we should do this sooner than later cc @alexanderbez @aaronc.

@aaronc
Copy link
Member

aaronc commented May 7, 2020

Sure 👍 . Why not just client.Context?

@alexanderbez
Copy link
Contributor Author

Absolutely!

@alessio
Copy link
Contributor

alessio commented May 7, 2020

Why not just client.Context

YES! please

@aaronc aaronc added this to the v0.39 milestone May 7, 2020
@sahith-narahari
Copy link
Contributor

I can take this up, it will also help us organize keys cli tests as @alessio suggested

@aaronc aaronc self-assigned this May 7, 2020
@aaronc aaronc assigned sahith-narahari and unassigned aaronc May 7, 2020
@alexanderbez
Copy link
Contributor Author

Thank you so much @sahith-narahari 🙏

@sahith-narahari
Copy link
Contributor

This is summary of discussion from discord
@alexanderbez proposed just renaming the CliContext to Context(will be imported as context.Context) but this is confusing as it collides with stdlib's Context which will be used once clients use gRPC

@aaronc suggested that the top level client package is for things that are likely to be imported a lot and CLIContext is currently imported a lot. so either we move some stuff that's in client/ elsewhere and/or we package client/ a top-level packages that aliases inner packages like the modules do

@alexanderbez
Copy link
Contributor Author

I would like to reiterate, grouping and packages should be done by context/domain and not by expected usage. However, clobbering with the stdlib context is a valid concern and may warrant moving Context types and logic to the root client package. As long as this is a painless effort, I'm onboard 👍

@alessio
Copy link
Contributor

alessio commented May 11, 2020

+1 for moving CLIContext into client

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants