#291: Save dependencies to clib manifest by default #299
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
exbotanical
commented
Oct 31, 2023
| command_option(&program, "-D", "--save-dev", | ||
| "save development dependency in clib.json or package.json", | ||
| setopt_savedev); | ||
| command_option(&program, "-N", "--no-save", |
There was a problem hiding this comment.
Not super sure about the shorthand flag. If commander supports passing NULL here, maybe we just allow the long option only?
There was a problem hiding this comment.
I opened a PR long time ago on commander to omit the short version. clibs/commander#25
I'll see if it still valid...
There was a problem hiding this comment.
@jwerle Cool, sounds good.
What's the standard procedure for clib - I push a tag (for this I'd bump the minor version) and merge? Thanks for the reviews!
jwerle
approved these changes
Nov 3, 2023
|
Looks good! |
diasbruno
approved these changes
Nov 3, 2023
stephenmathieson
approved these changes
Nov 8, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR modifies the behavior of
clib-installsuch that dependencies are saved to the clib manifest (clib.json/package.json) by default, mirroring the behavior of npm >= 5. It accomplishes this by deprecating the--saveoption and adding an additional flag--no-savewhich can be passed to prevent the dependency from being written to the clib manifest.Note I've retained the
--save-devoption so users can specify they want the dependency to save under dev dependencies. This is also aligned with the behavior of npm >= 5.Changes:
--saveoption and add a logger warning. Behavior when this flag is passed is effectively the same to prevent backward compatibility issues.--no-saveflag, which changes the install behavior such that the dependency is not added to the clib manifest.maketarget for the binaries as a dependency of thetesttarget. I noticed that if you runmake testwithout having runmakefirst, the tests will fail.--no-save. Also updated existing test cases where necessary with--no-savejust to maintain parity with what was there before. We could probably refactor the--saveand--no-savetests to use a common function, but I wanted to keep the PR small, and having separate test files lets us fine-tune the behavior for one without clobbering the other.Resolves #291.
When we're good, I can push a tag to bump the version or however you prefer. Happy to make any suggested changes.