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

Removes Cli.getInstance() #5707

Closed

Conversation

waldekmastykarz
Copy link
Member

@waldekmastykarz waldekmastykarz commented Dec 3, 2023

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 lowercase cli.ts to indicate that it's not exporting a type anymore. One of the things that you might find odd is that inside cli.ts, we're referring to exported properties and functions with cli.prop instead of prop. This is necessary to allow mocking exported properties in tests.

@martinlingstuyl
Copy link
Contributor

Merge conflict alert @waldekmastykarz, probably due to your other PR on validating short options. Could you solve them?

@martinlingstuyl martinlingstuyl self-assigned this Dec 5, 2023
Copy link
Contributor

@martinlingstuyl martinlingstuyl left a 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!

src/cli/cli.ts Show resolved Hide resolved
src/cli/cli.spec.ts Show resolved Hide resolved
@waldekmastykarz
Copy link
Member Author

Merge conflict alert @waldekmastykarz, probably due to your other PR on validating short options. Could you solve them?

Will do!

@martinlingstuyl
Copy link
Contributor

Hi @waldekmastykarz, if you fix the merge conflicts, I'll be able to merge soon.

Copy link
Contributor

@martinlingstuyl martinlingstuyl left a 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!

@martinlingstuyl
Copy link
Contributor

Merged manually, thank you 🥳

@waldekmastykarz waldekmastykarz deleted the remove-cli-instance branch December 11, 2023 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants