Skip to content
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 remove packages #3842

Merged
merged 3 commits into from
Apr 24, 2015
Merged

Conversation

jonathanKingston
Copy link
Member

Issue: #3841

@jonathanKingston
Copy link
Member Author

@rwjblue I'm pretty sure those timeouts mentioned in the appveyor ci are not relevant. What do you think to the API anyway?

@@ -894,6 +894,60 @@ Blueprint.prototype.addPackagesToProject = function(packages) {
};

/**
Used to remove a package from the projects `package.json`.

Generally, this would be done from the `afterInstall` hook, to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this description is a copy-past of the addPackage? Could you fix that please.

@abuiles
Copy link
Member

abuiles commented Apr 13, 2015

@jonathanKingston thanks! can you please address some of the comments?

@jonathanKingston
Copy link
Member Author

@abuiles working on these now sorry for missing those.

@jonathanKingston jonathanKingston force-pushed the add-remove-packages branch 2 times, most recently from 58df95b to 331adc2 Compare April 13, 2015 22:55
@jonathanKingston
Copy link
Member Author

@abuiles do these smoke tests cause issues on other pulls? I added in another test for the singular case to pass the new coveralls.

@kellyselden
Copy link
Member

@jonathanKingston Yes, the AppVeyor build currently has issues.

@jonathanKingston
Copy link
Member Author

@kellyselden +1 thanks for the quick response.

/**
Used to remove multiple packages from the projects `package.json`.

Generally, this would be done from the `afterInstall` hook, to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please fix the description, the following: ensure that a package that is required by a given blueprint is is not really what this is for, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abuiles yup updated sorry for the back and forwards with this, it's not acceptable I am sorry for that.

abuiles added a commit that referenced this pull request Apr 24, 2015
@abuiles abuiles merged commit 403b0cc into ember-cli:master Apr 24, 2015
@abuiles
Copy link
Member

abuiles commented Apr 24, 2015

Thanks!

@jonathanKingston
Copy link
Member Author

Thanks for your help with this 👍

@jonathanKingston jonathanKingston deleted the add-remove-packages branch April 27, 2015 01:29
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.

None yet

3 participants