Skip to content

Conversation

@AhmedOsman101
Copy link

@AhmedOsman101 AhmedOsman101 commented May 29, 2025

No description provided.

Updated Files 2025-05-29@10:01PM
@ranger6
Copy link
Collaborator

ranger6 commented May 30, 2025

Hello Ahmed. Thanks for your interest in the semver-tool.

I have a few comments:

  1. Your pull request has no commentary explaining what problem it solves, use cases, obvious bug to be fixed. The code change is simple to understand, but the only hint about motivation is the title mentioning "gracefully". From the code change, I assume that "gracefully" means "returns a zero exit code". It is true that a non-zero exit code may cause surrounding code to exit immediately, exit with an error, or ...
  2. A non-zero exit code means (generally) that there was an error. Not passing arguments to semver is an error. Your code still prints out the usage message explaining the correct usage. So, it seems appropriate that since there is an error, one should not mislead surrounding software by indicating a non-error (zero exit code).
  3. For interactive use, the error code rarely matters and a user can read the usage information. In a script--where semver is usually used--the error code is important.
  4. By returning zero on a error, the error is likely to be "swallowed". The surrounding code continues with bad data (in this case, no version string produced). Maybe it will crash later. Maybe a "blank version" will get checked in by the CI system. Who knows. A simple typo (hit "delete" in the editor, erasing the arguments) generates a potentially hard to find bug.
  5. It is "unfortunate" that the first part of the error message ("Unknown command:") gets sent to stdout rather than stderr. This has been the semver-tool API for years and years. It would (perhaps?) be better if nothing were written to stdout and a better error message (e.g. "no arguments provided") was sent to stderr. Note that because of the non-zero return, surrounding software is informed that the stdout should not be used as a version string! Anyone is free to file an "issue" about this so it can be further discussed.
  6. Your change causes the tests to fail (glad to see that the tests caught it). Moreover, any change request that changes behavior needs to come with tests written that exercise the change.
  7. Finally, the proposed change would result in a new major version for semver-tool: it makes breaking changes to the API. As the semver-tool is a mature and quite stable piece of software, a major change would have to bring major benefits or resolve a major issue.

-- robert

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