BurntSushi / cargo-benchcmp

A small utility to compare Rust micro-benchmarks.
The Unlicense
342 stars 21 forks source link

Project co-ownership? #6

Closed Apanatshka closed 8 years ago

Apanatshka commented 8 years ago

Ok, I've rewritten this issue because the original had petty complaints about things I should have expected anyway.


Are you interested in sharing project ownership with me? I have more things I'd love to contribute to the tool to make it even more useful. And I think I can learn a lot from you about Rust. I'm also willing to look into the other issues that are open right now. But I'd like to have a clear sense of the roles and the way we work together.

BurntSushi commented 8 years ago

I saw your original comment via email, and I didn't think it was petty at all! They were all valid criticisms and I really apologize for the pain I've caused. First and foremost, I added your name back to the authors list in commit 56cc221e195355aff8a5dd4c8eb4faceb3ec5820. It was unintentionally removed because I was stupid and careless when copying the top most block from the Cargo.toml of another project. Sorry about that!

Second, I'm sorry about the reddit announcement too. I know it can be a huge drag to have someone suck the wind out of you like that. My thought process was basically, "@Apanatshka rewrote cargo-benchcmp in Rust. Awesome! Tell the world!" I never stopped to think (or ask) that you might want to be the one to announce the changes which I would have been totally cool with.

OK, about the refactor, this is the messiest and is a reflection of my failure as a code reviewer. The problem was that I accepted a PR that probably wasn't quite ready yet, and I didn't really realize it until I actually tried using the tool after I merged the PR. There were several inconsistencies between the Rust rewrite and the original tool that I wanted to have fixed (we can talk about them if you like), and it didn't feel right to revert the merge and send it back to you, so I just moved ahead to fix them. The code refactor became a lot more extensive than I intended at the outset. Basically, my thought process here is that if I'm accepting a PR into a repo I maintain, then I must burden the responsibility of maintaining it. I've made the mistake in the past of accepting PRs that I couldn't maintain and it always results in sadness. I really wanted the Rust rewrite, so I decided to put in the work to bring your rewrite up to the point that I was happy maintaining it.


OK, with that said, on to your actual question... Given my prior communication failures, it's probably best to be super explicit here: when you're talking about project ownership, you're referring specifically to commit access and crates.io access, yes? My initial reaction to that is that I don't know whether we have a shared vision on where this project should go from here (which is partially my fault because I've never actually outlined a vision---the space at which this repo turned from a stupid-dumb-utility into a collaboration between two people has been incredibly small), which makes me apprehensive about sharing ownership. My vision at this point is roughly:

  1. Add tests (#3). (This was one of my goals with the refactor, specifically by compartmentalizing more of the logic, but I ran out of time to actually add tests.)
  2. Add better math (#4), specifically, take a sampling based approach and report p-values/confidence intervals.

Before sharing project ownership, I think it might be better to try and collaborate more. I'm happy to continue to do so, and I'll do my best to improve the code review process. To help keep expectations clear, I do think it's really important that we discuss significant changes before you do the work to send the PR. Again, I'm sorry about all of this and I'll do my best to make it right between us going forward. Thank you again for the Rust rewrite. :-)

Apanatshka commented 8 years ago

I think you've addressed all the original things I mentioned. It's good to know that the author thing was a mistake. The other things are also bad communication on my part. I removed a lot of that in my edit of the issue because I could imagine things making sense from your side.

As for merging the PR early, next time revert if you like. I'll get over it. I already got a lot out of your code review which helps me write good Rust code faster.

Anyway, I was happy to see your response. Let's leave this behind use :-)


So, I actually meant ownership more in spirit than in access. I'm fine with the vision you sketched so far, but shared ownership would mean discussing every change to that vision and both opening PRs rather than committing to master directly. This idea mostly came out of seeing your refactor on master and thinking "darn, now I have to on rebase that". Perhaps the co-ownership isn't really necessary as long as I take your advice and discuss planned work in an issue first ^^ Let's start with that and see how it goes.

BurntSushi commented 8 years ago

So, I actually meant ownership more in spirit than in access. I'm fine with the vision you sketched so far, but shared ownership would mean discussing every change to that vision and both opening PRs rather than committing to master directly. This idea mostly came out of seeing your refactor on master and thinking "darn, now I have to on rebase that". Perhaps the co-ownership isn't really necessary as long as I take your advice and discuss planned work in an issue first ^^ Let's start with that and see how it goes.

Ah, makes sense now. I typically don't like to do this for small projects because there's so much overhead involved, but I am very happy to compromise here and say that I'll submit any PRs for non-trivial changes in the future. I'll still push to master directly though for small commits like version bumps (because I have a script that does it automatically), but those should be easy to rebase against.

And yeah, rebasing your old work against my revisions is probably nasty. If it were me, I'd probably bolt your new stuff on top of the new code instead of trying to recover a failed merge. I really should have submitted a PR for my refactor, you're right. Ug. So sorry. :-(

Apanatshka commented 8 years ago

Don't worry, everyone commits to master once in a while when they shouldn't :) I'll probably do that, skip Git and just copy stuff over / reimplement directly. Half of that feature branch was a mess anyway, with refactoring code and zeroing in on a good enough cli interface.. And before I start, I'm going to open an issue and see what you think ;)