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

Merge pidtree back into ps-tree? #30

Open
simonepri opened this issue Mar 16, 2018 · 26 comments
Open

Merge pidtree back into ps-tree? #30

simonepri opened this issue Mar 16, 2018 · 26 comments

Comments

@simonepri
Copy link
Collaborator

simonepri commented Mar 16, 2018

Following this old issue:
#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?
#18 (comment)

cc @zixia @soyuka

@soyuka
Copy link

soyuka commented Mar 16, 2018

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

@simonepri
Copy link
Collaborator Author

simonepri commented Mar 16, 2018

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
Copy link

soyuka commented Mar 16, 2018

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
Copy link
Collaborator Author

simonepri commented Mar 16, 2018

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.
#17

@huan
Copy link
Contributor

huan commented Mar 16, 2018

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

:-/

@simonepri
Copy link
Collaborator Author

simonepri commented Mar 16, 2018

@zixia can you open another PR removing the addition of appveyor and the changes of the readme?
We have already #29 for that.

@huan
Copy link
Contributor

huan commented Mar 16, 2018

@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
Copy link
Collaborator Author

Thank you! I've commentend under the PR.

@simonepri
Copy link
Collaborator Author

@indexzero See also this
#28 (comment)

@soyuka
Copy link

soyuka commented Mar 16, 2018

@simonepri
Copy link
Collaborator Author

@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
Copy link

soyuka commented Mar 16, 2018

@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
Copy link
Collaborator Author

@nelsonic @indexzero news?

@soyuka
Copy link

soyuka commented Mar 18, 2018

@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
Copy link
Collaborator Author

simonepri commented Mar 18, 2018

Actually nelsonic has already merged some of my PRs some days ago:
#26
#25

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

@huan
Copy link
Contributor

huan commented Mar 19, 2018

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

@simonepri
Copy link
Collaborator Author

simonepri commented Mar 19, 2018

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:
soyuka/pidusage#49

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

@huan
Copy link
Contributor

huan commented Mar 19, 2018

@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
Copy link
Owner

@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
Copy link
Owner

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

@indexzero indexzero changed the title Who are the maintainers? Merge pidtree back into ps-tree? Nov 26, 2018
@simonepri
Copy link
Collaborator Author

simonepri commented Nov 26, 2018

@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
Copy link
Owner

indexzero commented Nov 27, 2018

@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
Copy link
Contributor

huan commented Nov 27, 2018

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

@simonepri
Copy link
Collaborator Author

@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
Copy link
Owner

indexzero commented Nov 28, 2018

@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
Copy link
Owner

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.

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

No branches or pull requests

5 participants
@indexzero @soyuka @huan @simonepri and others