-
Notifications
You must be signed in to change notification settings - Fork 131
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
Add uninstall command #1111
Add uninstall command #1111
Conversation
@@ -536,6 +554,10 @@ main = do | |||
env' <- runSpago env (mkBuildEnv buildArgs dependencies) | |||
let options = { depsOnly: true, pursArgs: List.toUnfoldable args.pursArgs, jsonErrors: false } | |||
runSpago env' (Build.run options) | |||
Uninstall { packagesToRemove, selectedPackage, testDeps } -> do | |||
{ env, fetchOpts } <- mkFetchEnv offline { packages: packagesToRemove, selectedPackage, ensureRanges: false, testDeps: false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The packages
here are additional packages to add to the config, which is the opposite of what we want to achieve here 😄
{ env, fetchOpts } <- mkFetchEnv offline { packages: packagesToRemove, selectedPackage, ensureRanges: false, testDeps: false } | |
{ env, fetchOpts } <- mkFetchEnv offline { packages: [], selectedPackage, ensureRanges: false, testDeps: false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...are additional packages to add to the config
AFAICT via mkFetchEnv
, that's not true. Rather, we just validate there that the strings we got from the CLI can be parsed into PackageName
values. The resulting packageNames
value is never used anywhere else and is just stored in fetchOpts
. And for context, the only reason why I'm using mkFetchEnv
instead of doing this outside of that is because I need a workspace
value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah sorry - I got confused about mkFetchEnv
vs Fetch.run
, all good
src/Spago/Command/Uninstall.purs
Outdated
editSpagoYaml | ||
:: forall m | ||
. MonadAff m | ||
=> { configPath :: FilePath | ||
, modifyConfig :: Core.Config -> Core.Config | ||
, onError :: String -> m Unit | ||
} | ||
-> m Unit | ||
editSpagoYaml { configPath, modifyConfig, onError } = do | ||
content <- liftAff $ FS.readYamlDocFile Core.configCodec configPath | ||
case content of | ||
Left err -> | ||
onError err | ||
Right { yaml: config } -> | ||
liftAff $ FS.writeYamlFile Core.configCodec configPath $ modifyConfig config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This piece of code was fine for tests, but it's not good to use for normal business - rewriting the config like that loses comments and extra fields the config might contain.
We should instead manipulate the Yaml.Doc
, and use the same setup as it's done in the Config module, where we get the Doc
from the workspace, and apply an FFI function that does the yaml manipulation while preserving comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok. Let me fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Description of the change
Fixes #1091.
Checklist:
README
P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊