Skip to content
This repository was archived by the owner on Aug 30, 2021. It is now read-only.

Conversation

@rhutchison
Copy link
Contributor

No description provided.

@ilanbiala
Copy link
Member

@rhutchison this should wait until 0.4.0 is released because we don't need this to release. In fact, I'd rather try to sort out whether we are going to support Grunt or Gulp before doing this, because there is no point in having support for both as we just need to maintain more.

@rhutchison
Copy link
Contributor Author

@ilanbiala are you in the gitter chat? This PR is against your 4.0 branch. Should update dependencies and then figure out gulp vs grunt. Some of these dependencies are very old and if you are releasing 4.0, it should be tested against newer libs.

@rhutchison
Copy link
Contributor Author

Resolves #612

@lirantal
Copy link
Member

lirantal commented Jul 6, 2015

@rhutchison we'll get to review this PR soon, no worries.

@rhutchison rhutchison mentioned this pull request Jul 7, 2015
@lirantal
Copy link
Member

lirantal commented Jul 7, 2015

hi @rhutchison I want to merge this but there's a bit of a mess there with the commit messages like reverts and some other "mistake commits" as well as some space indentation that is wrong (in the package.json file for example) and some spaces that were added but aren't really a change.

I don't want to merge with the current commit history, can you please fix this up quickly and squash your commits to logical commits that make sense with the changes?

Please hurry doing so as I want to merge this in to master_to_0.4.0 and quickly after merge that one into 0.4.0.

Copy link
Member

Choose a reason for hiding this comment

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

is that dropdown text suppose to be there like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's how it works with ui-bootstrap.

@rhutchison
Copy link
Contributor Author

@lirantal package.json is written to by npm --save and --save-dev

Please see npm/npm#4718

I updated .editorconfig - Gym@73be794

@lirantal
Copy link
Member

lirantal commented Jul 8, 2015

@rhutchison with regards to package.json, you re-ordered all of the packages so the diff looks like this now:

image

and it's hard to figure out which package you added, or removed, or just updated the version for because they are all re-organized again in the new commit.
Also, was there a specific reason for updating the dependencies?

@ilanbiala WDYT?

@simison
Copy link
Contributor

simison commented Jul 8, 2015

@rhutchison @lirantal I think we should let the package.json re-organising and spacing go trough now and it'll be easier in the future. When working with package.json trough command line, that's what happens. If we don't let this ugly diff go trough here now, people will have it ahead of them when they'll fork and add/remove packages at their version.

@ilanbiala
Copy link
Member

@lirantal the package.json reordering makes sense, but right now it definitely makes it harder to diff. I say we update deps using https://david-dm.org/ and then allow npm to format the package.json in a different PR.

@rhutchison
Copy link
Contributor Author

npm is responsible for sorting the packages alphabetically. Here is the list of updates prior to applying this commit.

2015-07-08 10_22_18-administrator_ command prompt

@ilanbiala
Copy link
Member

@rhutchison I'm not saying that npm isn't doing something logical, I'm saying that we should manually increment the versions based on david-dm and let npm format the package.json properly in a different commit or even PR, because that will simplify reviewing.

@lirantal
Copy link
Member

lirantal commented Jul 8, 2015

Ok then, I'll go ahead ahead and merge this in, we can indeed resolve the package.json later.

lirantal added a commit that referenced this pull request Jul 8, 2015
@lirantal lirantal merged commit 99e0ea8 into meanjs:master_to_0.4.0 Jul 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants