Skip to content

Conversation

@PeterDaveHello
Copy link
Collaborator

No description provided.

@thlorenz
Copy link
Owner

Please use ~ ranges, otherwise I can't accept this.

@PeterDaveHello
Copy link
Collaborator Author

@thlorenz is there any concern about using ^? Thanks.

@thlorenz
Copy link
Owner

Yes, opens up to more breakage as people don't perfectly follow semver. I prefer to be more conservative and therefore would like to stick to ~.
If you don't agree, fine, but I won't merge this since I don't wanna deal with lots of breakage issues in the future.

Thanks.

@PeterDaveHello
Copy link
Collaborator Author

So do we need to change the other dependencies also to use ~? Thanks.

@thlorenz
Copy link
Owner

Ideally yes, wasn't aware that some of them had a ^ at this point.
Most likely got introduced with another PR and I missed it :)

@PeterDaveHello
Copy link
Collaborator Author

I add a commit to use ~ in all the dependencies now

Copy link
Owner

@thlorenz thlorenz left a comment

Choose a reason for hiding this comment

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

Sorry about the confusion, but of course my own repos can have ^ since I follow semver.

Also please verify that underscore's version makes sense. >= basically means we were on the latest, so we should stay on the latest.

Thanks a lot for your patience and again sorry for the confusion.

package.json Outdated
"htmlparser2": "~3.9.2",
"markdown-to-ast": "~3.4.0",
"minimist": "~1.2.0",
"underscore": "~1.3.3",
Copy link
Owner

Choose a reason for hiding this comment

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

what's the latest underscore? We need to make sure we don't downgrade the version here.
Should be ~<whatever latest version is>.

package.json Outdated
"minimist": "~1.1.0",
"underscore": ">=1.3.3",
"update-section": "^0.3.0"
"anchor-markdown-header": "~0.5.5",
Copy link
Owner

Choose a reason for hiding this comment

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

My repo, so we can use ^ here (I trust myself to follow semver ;) )

package.json Outdated
"markdown-to-ast": "~3.4.0",
"minimist": "~1.2.0",
"underscore": "~1.3.3",
"update-section": "~0.3.0"
Copy link
Owner

Choose a reason for hiding this comment

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

My repo, so we can use ^ here (I trust myself to follow semver ;) )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

@thlorenz
Copy link
Owner

Awesome, many thanks :)
Please squash into one commit first and then merge to master.

I made you collaborator.
So when you merge please follow this guide.
I will then version and publish to npm.

Thanks.

@PeterDaveHello
Copy link
Collaborator Author

Hi @thlorenz, IMO, these two commits are doing different their own task, one for version bump and one for syntax, can we just leave them be separated? Thanks.

@thlorenz
Copy link
Owner

@PeterDaveHello yeah, makes sense. Sounds good to me, go ahead and merge then.

@PeterDaveHello
Copy link
Collaborator Author

@thlorenz I'm confused about how should I merge it since the reference doc says not to use the green button, this PR branch is up to date, so can I merge it right here? Thanks.

@thlorenz
Copy link
Owner

Merge using git on the terminal, i.e. you'd do:

git rebase master
git co master
git merge dependencies-upgrade

It's guaranteed to be fast forward merge since you are rebasing first and if you have any conflicts you get to fix them right there.
I'm mentioning alternative ways to fast forward merges, including some links to more details here

@thlorenz
Copy link
Owner

You could also just hit the Rebase and merge option in the button dropdown, but learning to do this on the terminal is a good thing to do in general.

@PeterDaveHello PeterDaveHello merged commit c3aa65f into thlorenz:master Feb 27, 2017
@PeterDaveHello PeterDaveHello deleted the dependencies-upgrade branch February 27, 2017 21:53
@PeterDaveHello
Copy link
Collaborator Author

@thlorenz Actually I totally have no problem on how to merging locally, just want to make sure the policy and procedure, if I merge it locally, will this PR also shown as "Merged"? Or it or just be closed? Thanks.

@thlorenz
Copy link
Owner

Thanks went out as 1.3 .. I did a minor upgrade since we changed a gzillion deps.

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.

2 participants