indexzero / ps-tree

MIT License
149 stars 29 forks source link

Merge pidtree back into ps-tree? #30

Open simonepri opened 6 years ago

simonepri commented 6 years ago

Following this old issue: https://github.com/indexzero/ps-tree/issues/10

Who are the current maintainers of this package? For what I saw: @indexzero is the author (can push on the repo and can publish on npm) @nelsonic is one of the maintainers (can only push on the repo) Am I missing someone?

Can we add more contributors to this repo? https://github.com/indexzero/ps-tree/issues/18#issuecomment-373294868

cc @zixia @soyuka

soyuka commented 6 years ago

We maybe could release a community fork? I've a pending fix on @zixia 's fork also haha

simonepri commented 6 years ago

Let's see what @nelsonic says about that. But this package has 4 Mln downloads per month so if we can keep working on this repo would be better for everyone.

Actually the only one that now can push on npm is @indexzero and he have said that he don't want to maintain this anymore.

@soyuka are you referring to this https://github.com/zixia/ps-tree/pull/1? Can you create the PR on this repo too?

soyuka commented 6 years ago

Can you create the PR on this repo too?

It's based on @zixia ´s work on the repository so I guess we need to merge his changes first :|

simonepri commented 6 years ago

Oh right! So let's make a single PR with both changes.

I missed that because @zixia has done all the work in this single PR. https://github.com/indexzero/ps-tree/pull/17

huan commented 6 years ago

@soyuka Let me merge your change in my PR, then I believe my PR at here will include your PR....

:-/

simonepri commented 6 years ago

@zixia can you open another PR removing the addition of appveyor and the changes of the readme? We have already https://github.com/indexzero/ps-tree/pull/29 for that.

huan commented 6 years ago

@simonepri Ok no problem.

I had just modified my PR and remove the README&Appveyor changes.

Please let me know if you think I should modify other parts as well.

simonepri commented 6 years ago

Thank you! I've commentend under the PR.

simonepri commented 6 years ago

@indexzero See also this https://github.com/indexzero/ps-tree/pull/28#issuecomment-373663239

soyuka commented 6 years ago

FTR merged #17 #24 #27 and #28 to https://github.com/soyuka/ps-tree

simonepri commented 6 years ago

@soyuka Lets wait @nelsonic before proceed please.

I don't see any advantage of working on a community fork if we can push on npm from this one.

soyuka commented 6 years ago

@simonepri I don't want to bother anyone ;) I'm just using this in pidusage-tree, and therefore will use my fork for the time being :). Obviously if things gets handled here I'll use the original version.

https://twitter.com/andrestaltz/status/961600427832872961

TL;DR

  • Publish forked packages
  • Inform, don't annoy, the original author
  • Open source is pointless unless for reading the source
  • Less "githubness", more openness
  • Small libraries!
  • Disagreement is okay, it's great actually
  • Minimize consensus surface
simonepri commented 6 years ago

@nelsonic @indexzero news?

soyuka commented 6 years ago

@simonepri pinging them is useless just let them answer if they want to I'm sure they've been notified by now ;). Note that my last contributions to this were almost 1 year ago and there has been no answer yet.

simonepri commented 6 years ago

Actually nelsonic has already merged some of my PRs some days ago: https://github.com/indexzero/ps-tree/pull/26 https://github.com/indexzero/ps-tree/pull/25

So I don't think is useless. They are alive. 😁

huan commented 6 years ago

But it seems that @nelsonic only merge Badges...

simonepri commented 6 years ago

Actually I've discovered that the algorithm inside this package is wrong: https://github.com/indexzero/ps-tree/blob/master/index.js#L79

Following the suggestion of soyuka I've re-created the package using the same codebase developed in: https://github.com/soyuka/pidusage/pull/49

You can find it here: https://github.com/simonepri/pidtree

huan commented 6 years ago

@simonepri I saw your pidtree npm, and thanks for adding me to the collaborators.

It seems we have to do a hard fork for this module finally...

indexzero commented 5 years ago

@huan @simonepri if you'd like to discuss merging back into a ps-tree@2.0.0 API please let me know. My apologies for not responding sooner. This was on my to-do list, but kept getting pushed back.

indexzero commented 5 years ago

Updating the title of this issue to reflect the current discussion.

simonepri commented 5 years ago

@indexzero pidtree is a from scratch re-work of this package, I'm not 100% sure that having it merged back here is the right choice. @soyuka

indexzero commented 5 years ago

@simonepri exactly! And it has an improved algorithm that is more correct.

This module is downloaded about 5,000,000 times a month. Wouldn't it be great if all those folks got a better quality module?

By releasing it as 2.0.0 it gives folks an upgrade path that is opt-in as well.

If you are interested I would add you as an owner and a maintainer on npm. Let me know! ^_^

huan commented 5 years ago

@indexzero it's great to see you wake up and come back to us! :D

simonepri commented 5 years ago

@indexzero Your point is right. I'm still not 100% happy with the implementation of pidtree and I would like to do some rework before starting to think about merging it back here.

My idea is to create separate packages for each platform and then to have pidtree or ps-tree to be just a wrapper for the right implementation. Also, I wold like to add a more sophisticated cross-platform testing though docker.

The ETA of this would be at least a month, since I'm super busy right now.

indexzero commented 5 years ago

@simonepri that makes sense. Given your past excellent work on PR #24, #27, and #29 I've added you as a collaborator to this project. When you accept that invite I am happy to make you a full Administrator.

screen shot 2018-11-28 at 4 08 31 pm

Once you've reached a place you're happy with from an implementation perspective please let me know and I will add you as an owner on npm.

indexzero commented 5 years ago

So it turns out that for personal repositories they only the person they are associated with can be administrators. I am going to move this over to the foreverjs org since it's heavily depended on by that package –– unless there are other thoughts of course.