-
Notifications
You must be signed in to change notification settings - Fork 21
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
Added update command and rewritten install command #93
Conversation
Modified lib/promise/update.js Add json/help-update.json
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.
indentation needs a bit of a sort-out...
Just a reminder to ensure that the |
@moloko, @tomgreenfield install command now runs with untagged plugins. Is there anything else needed before requesting +1s? |
@chris-steele just a bit of an indentation sort out if you don't mind? currently a mix of tabs & spaces |
@moloko checked update and install scripts - they're all using space indentation as far as I can see. Did you mean globally rather than these two scripts? |
as far as I can see all the files in this PR are using a mix of tabs and spaces - sometimes in the same line! I'll mark a few with comments so you can see what I mean. |
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.
hope this helps show what I mean
lib/commands/install/InstallLog.js
Outdated
logProgress:function(str) { | ||
lastProgressStr = str; | ||
isLoggingProgress = true; | ||
readline.cursorTo(process.stdout, 0); |
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.
tab indentation
lastProgressStr = str; | ||
isLoggingProgress = true; | ||
readline.cursorTo(process.stdout, 0); | ||
process.stdout.write(str); |
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.
space indentation
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.
@moloko hopefully caught all the files I've edited in this PR
|
||
Plugin.call(this, ep.name || ep.source, version); | ||
|
||
if (this.version !== any) { |
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.
tabs AND spaces!
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.
@moloko sorry I was literally just looking at the big two files in this PR (install and update) and forgot about the dependencies. Is there a preference - tabs or spaces?
Changes in this PR:
New command
update
--check
allows user to see what updates are availableCommand
install
rewritten--compatible
ensures only the latest compatible plugins are installed--dry-run
allows user to see what the command will do without making any changesRationale for
install
rewriteIf the master version of manifest (
adapt.json
) is kept up-to-date then appropriate versions will be installed whenadapt install
is run in a new project. When installing an older version of the framework (for testing purposes, for legacy plugin/browser support etc) the manifest may not be appropriate and so incompatible plugins may be installed. Manually determining appropriate versions of plugins for a project is tedious and error prone (and equally for the master version).With these changes the manifest is still respected; so if a plugin is constrained to a semver then the command will attempt to install an applicable version. If '*' is specified then the latest compatible version is installed.
A consequence of these changes is that it will be no longer necessary to amend the master version of the manifest and we can return to using the wildcard '*'.
Notes
Install the prerelease that contains these changes by doing
npm install -g adapt-cli@next