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.

Sign up for GitHub

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

Jump to bottom

fix!: exit success when adding an already-added plugin #1598

Merged
merged 1 commit into from
Jul 26, 2023
Merged

fix!: exit success when adding an already-added plugin #1598

merged 1 commit into from
Jul 26, 2023

Conversation

hyperupcall
Copy link
Contributor

Summary

Closes #841

I do not consider this a breaking change because the fixed behavior does not break currently existing scripts that have the workaround (ie (exit 2) || : has the same affect as (exit 0) || :)

@hyperupcall hyperupcall requested a review from a team as a code owner July 14, 2023 22:56
@hyperupcall hyperupcall force-pushed the hyperupcall-fix-plugin-add-exit-code branch from 9896680 to 4858c5b Compare July 14, 2023 23:05
@hyperupcall hyperupcall mentioned this pull request Jul 15, 2023
Merged
@hyperupcall hyperupcall force-pushed the hyperupcall-fix-plugin-add-exit-code branch from 4858c5b to 47c7679 Compare July 18, 2023 05:04
@jthegedus
Copy link
Contributor

jthegedus commented Jul 26, 2023 •
edited
Loading

I agree with this change.

This is a breaking change as people have scripts dependent on the output here. It was added specifically because people wanted to know the difference of these return statuses.

I am updating the PR title conventional commit to reflect this

-fix:
+fix!:

I am open to further discussion here, but I find, for the most part, OSS libraries do a bad job of determining what is considered a public API and therefore what is considered "breaking". Exit codes and formatting of our return values are our public API, so I consider this type of change breaking.

Whilst guards such as (exit 2) || : should be used when consuming the return code, they may not be and that is somewhat on us. Despite being pre 1.0.0 and semver technically allowing us to break everything as much as we want, we should indicate to external users as best as possible. IMO this means this should be considered breaking.

@jthegedus jthegedus changed the title fix: Exit success when adding an already-added plugin fix!: exit success when adding an already-added plugin Jul 26, 2023
jthegedus
jthegedus approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@jthegedus jthegedus left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM. Finally! 😌

@jthegedus jthegedus merged commit 4dd1904 into asdf-vm:master Jul 26, 2023
@hyperupcall hyperupcall deleted the hyperupcall-fix-plugin-add-exit-code branch July 26, 2023 11:42
@p-linnane p-linnane mentioned this pull request Sep 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tony-sol tony-sol tony-sol left review comments

@jthegedus jthegedus jthegedus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

asdf plugin add should exit code 0 in most cases
3 participants
@hyperupcall @jthegedus @tony-sol