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

[PDE-4682] chore(cli): Add @clif/core dependency, upgrade @oclif/plugin-help #739

Merged
merged 3 commits into from
Feb 2, 2024

Conversation

rnegron
Copy link
Member

@rnegron rnegron commented Jan 30, 2024

As mentioned in #556 (comment), simply upgrading @oclif/plugin-help will break CLI functionality due to certain helper methods we were using in ZapierBaseCommand being removed. In fact, they were moved over to the @oclif/core library.

This PR adds the @oclif/core dependency, which is also the first step in migrating over from deprecated helper libraries.

A visual difference in how text is formatted can be observed before and after this PR. I'm looking into why this formatting change occurred to see if we can maintain the previous formatting style.

Before (wider terminal):

Screenshot 2024-01-30 at 3 55 57 PM

After (wider terminal):

Screenshot 2024-01-30 at 3 55 47 PM

Before (smaller terminal):

Screenshot 2024-01-30 at 3 56 07 PM

After (smaller terminal):

Screenshot 2024-01-30 at 3 56 17 PM

@rnegron rnegron marked this pull request as ready for review February 1, 2024 19:54
@rnegron
Copy link
Member Author

rnegron commented Feb 2, 2024

Opening for review because I'm not sure it's worth the time to implement changes to make the renderList display as before. The display does look uglier... but I think the tradeoff of closing #555 is worth it.

The reason why the display changes is because the renderList implementation in @oclif/core is different: https://github.com/oclif/core/blob/10b7dc0dd207906df208b0c5b85e309106dd8a5f/src/cli-ux/list.ts#L15, it does not include additional properties like the one from @oclif/plugin-help (which no longer exists after the upgrade).

Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

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

@rnegron I found instead of importing renderList from @oclif/core/lib/cli-ux/list, we should it from @oclif/help/lib/list. That way we can keep using the same renderList with previous options. Turns out @oclif/plugin-help was not gone but was renamed to @oclif/help.

I created #743 to fix. Otherwise, this PR looks good to me. Feel free to find someone else to approve. Thanks!

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.

2 participants