-
Notifications
You must be signed in to change notification settings - Fork 332
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
Removes Cli.getInstance() #5707
Conversation
Merge conflict alert @waldekmastykarz, probably due to your other PR on validating short options. Could you solve them? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @waldekmastykarz, looks much better indeed!
Will do! |
Hi @waldekmastykarz, if you fix the merge conflicts, I'll be able to merge soon. |
928f73d
to
6be3ddf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks and works great!
Merged manually, thank you 🥳 |
Removes
Cli.getInstance()
and replaces it with a module setup.getInstance()
was odd for our needs and seems like a pattern we brought over from c#-like thinking.Most of the refactoring happens in
Cli.ts
which has been renamed to lowercasecli.ts
to indicate that it's not exporting a type anymore. One of the things that you might find odd is that insidecli.ts
, we're referring to exported properties and functions withcli.prop
instead ofprop
. This is necessary to allow mocking exported properties in tests.