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

support package-lock.json #954

Merged
merged 2 commits into from
May 28, 2017
Merged

Conversation

jens1o
Copy link
Member

@jens1o jens1o commented May 28, 2017

Fixes #953

Changes proposed:

  • Add
  • Prepare
  • Delete
  • Fix

Things I've done:

  • My pull request fixes an issue, I referenced the issue.

@jens1o
Copy link
Member Author

jens1o commented May 28, 2017

now, we have failing checks... Maybe we can provide a pr at chai-for-promises, so we can fix it?

@codecov
Copy link

codecov bot commented May 28, 2017

Codecov Report

Merging #954 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #954   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          44     44           
  Lines        2109   2109           
  Branches       99     99           
=====================================
  Hits         2109   2109
Impacted Files Coverage Δ
src/icon-manifest/supportedExtensions.ts 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d77382...a521ccd. Read the comment docs.

@JimiC
Copy link
Member

JimiC commented May 28, 2017

Until chai-as-promised updates, I wouldn't recommend it.

@jens1o
Copy link
Member Author

jens1o commented May 28, 2017

Sorry? I don't understand what you're trying to say.

@JimiC
Copy link
Member

JimiC commented May 28, 2017

Issue: chaijs/chai-as-promised#184
WIP-PR: chaijs/chai-as-promised#157

@JimiC
Copy link
Member

JimiC commented May 28, 2017

@jens1o I would appreciate if the PR you create are from your fork.

@jens1o
Copy link
Member Author

jens1o commented May 28, 2017

Yeah, I saw this just after I created it, sorry. Won't happen again.

@robertohuertasm
Copy link
Member

@JimiC, @jens1o, personally I don't care about where the PR is coming. I mean, in this repo we're all maintainers and as a team, in my opinion, working in the same repo is the way to go. There's no benefit in keeping a fork. This was invented in order to collaborate with repositories where you don't have any privilege. Once you're allowed to push to the repo you can create branches. Indeed, it's easier for the team members to collaborate with each other: if they need some help it's easier to add things to your colleague's branch. No need to add a remote for every member of your team if you want to know what are they working on or make some modifications to their suggested changes and so on.

That being said, I'm ok with you working on a fork if that's the way you prefer to work. No problem with that. I just wanted to point out that for me that wouldn't be the first option for working as a team.

@robertohuertasm robertohuertasm merged commit 5624ace into master May 28, 2017
@robertohuertasm robertohuertasm deleted the jens1o-support-packagelock.json branch May 28, 2017 13:16
@JimiC
Copy link
Member

JimiC commented May 28, 2017

@robertohuertasm Personally I like keeping the main repo clean. Imagine each collaborator creating a branch for each thing they are working on. It can easily become a mess. I've seen it happening and that's why I'm in favor working with fork/branch flow. On the contrast creating as many branches as they want on their own fork doesn't affect anyone.

@jens1o https://github.com/chaijs/chai/releases/tag/4.0.0

@robertohuertasm
Copy link
Member

@JimiC, I gently disagree. The rule of thumb is that feature branches should be kept local until ready to be merged and then, and only then, pushed to the remote repo. Then it's time for revision, merging and finally deleting the feature branch (via PR or MR depending on the Git service). You're aware that until a branch is not pushed it only exists in your local copy so you can create all the branches you need without cluttering the remote repo.

I've been working like this for a long time now and I've not found any kind of messes. But of course, these are just practices and everyone is entitled to do what they think is best for them. 😉 I just wanted to tell you my preference and give you the reasoning behind it, not trying to impose my vision to anybody.

PS: Just out of curiosity @JimiC, do every member of your team (at work) work with a fork of the main repo?

@jens1o
Copy link
Member Author

jens1o commented May 28, 2017

Personally, I also see the advantages, because when I do it here, I don't have to fetch the upstream, but I can work how I want without additional steps. Until today, I didn't knew how @robertohuertasm would react, that's why I kept it in my fork.

@JimiC
Copy link
Member

JimiC commented May 28, 2017

@robertohuertasm I was merely expressing a personal belief. Because I work on diff machines (at work and at home) I tend to push my branches to make them available from everywhere. This way I can continue to work on a feature, bugfix, concept wherever I am.

Regarding your P.S. question, every member takes up a project so it's basically a one-man team. So the Git-flow is somewhat indifferent.

@robertohuertasm robertohuertasm modified the milestone: 7.8.0 May 28, 2017
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.

Support package-lock.json generated by npm5
3 participants