Remove obsolete packages. #338
Conversation
lib/sync-settings.coffee
Outdated
| if redundant_packages.length is 0 | ||
| atom.notifications.addInfo "Sync-settings: no packages to remove" | ||
| return cb?() | ||
| else |
There was a problem hiding this comment.
In the else case the cb doesn't seem to get called anywhere?
lib/sync-settings.coffee
Outdated
| @@ -232,6 +232,7 @@ SyncSettings = | |||
| if atom.config.get('sync-settings.syncPackages') | |||
| callbackAsync = true | |||
| @installMissingPackages JSON.parse(file.content), cb | |||
| @uninstallRedundantPackages JSON.parse(file.content), cb | |||
There was a problem hiding this comment.
While this is a great feature I think it is not necessarily what all users want. Therefore I think this should be an option in the settings which defaults to false to maintain the current behavior.
lib/sync-settings.coffee
Outdated
| else | ||
| for pkg in redundant_packages | ||
| packageManager = new PackageManager() | ||
| packageManager.uninstall pkg |
There was a problem hiding this comment.
This call of uninstall should also pass a callback to inform the user about progress / success / failure of the command.
|
All comments addressed. Used some existing code so not very clean but at least works well. |
|
Seems travis-ci finally works. This is just a ping. |
lib/config.coffee
Outdated
| description: 'Packages installed but not in the backup will be removed when restoring backups' | ||
| type: 'boolean' | ||
| default: false | ||
| order: 11 |
There was a problem hiding this comment.
Is there a reason removePackage needs to be inserted here, thus pushing the order: attribute of everything that follows?
Could it have been appended to the end of the list instead?
|
It's an arbitrary position that I found may form a "block" of settings with similar functions. Will make another commit to have the change soon. |
|
@dirk-thomas I believe the |
|
@dirk-thomas Any news on this? I am also eagerly awaiting this feature. ;-) |
|
Looking forward to this. Many thanks. |
|
Any word on this? |
lib/config.coffee
Outdated
| @@ -67,4 +67,10 @@ module.exports = { | |||
| default: false | |||
| description: "Mute 'Latest backup is already applied' message" | |||
| order: 14 | |||
| removePackage: | |||
There was a problem hiding this comment.
Can you please rename this to removeObsoletePackages.
There was a problem hiding this comment.
Can you please add the trailing s to make it plural. Since the title is "the same" afterwards you can remove it (it is automatically generated by separating the capitalized words of the key).
lib/config.coffee
Outdated
| @@ -67,4 +67,10 @@ module.exports = { | |||
| default: false | |||
| description: "Mute 'Latest backup is already applied' message" | |||
| order: 14 | |||
| removePackage: | |||
| title: 'Remove unused packages' | |||
There was a problem hiding this comment.
Can you please update this to Remove obsolete packages. (The following description is good as it is I think.)
lib/sync-settings.coffee
Outdated
| @@ -300,6 +302,64 @@ SyncSettings = | |||
| console.debug "config.set #{keyPath[1...]}=#{value}" | |||
| atom.config.set keyPath[1...], value | |||
|
|
|||
| removeRedundantPackages: (packages, cb) -> | |||
| available_packages = @getPackages() | |||
| redundant_packages = [] | |||
There was a problem hiding this comment.
This should be obsolete_packages.
lib/sync-settings.coffee
Outdated
| @@ -300,6 +302,64 @@ SyncSettings = | |||
| console.debug "config.set #{keyPath[1...]}=#{value}" | |||
| atom.config.set keyPath[1...], value | |||
|
|
|||
| removeRedundantPackages: (packages, cb) -> | |||
There was a problem hiding this comment.
Can you please rename the function to removeObsoletePackages.
The argument packages is kind of ambiguous - if it refers to the redundant / obsolete packages. Therefore I would suggest renaming the argument to used_packages (or does anyone have a better proposal?) to clarify which package names it contains.
|
@James-Yu heads up. @dirk-thomas has asked for a change to a name. |
|
Extraneous? |
|
Here they are. |
|
@dirk-thomas back in your court |
|
Ping |
|
What's the status of this? |
|
@James-Yu Why did you close the pull request? |
…ckages to remaining_packages, rename available_package to keep_installed_package
Fixes #91 #305