I'm a small business programmer. I love solving tough problems with Python and PHP. If you like what you're seeing, you should probably follow me here on dev.to and then checkout my blog.
Hey Jason, I recommend you follow the instruction/rules of the project. If the project doesn't have any rules, you might email the maintainer and ask what they prefer before you spend your valuable free time writing a PR that's going to get rejected after a 30 second review, which is probably what's going to happen to your PR.
In the absence of specific advice from the maintainer, I would follow my own advice in this post. PRs need to be easy to review. They need to be small enough that your brain doesn't give it before you understand the changes and make sure it's okay.
Since I wrote this post we've lowered the time limit for reviews to 40 minutes. My team rejects PRs when we hit the 40 minute mark--"too long" is the feedback the author gets. But that's our limit. The sweet spot seems to be around 20 minutes.
So how would we handle your situation? Chained PRs.
The first PR is just formatting the whole project in one commit with the message: "ran the project through the format -no manual code changes."
Then we'd branch off of that and create one or more stories to refactor the code where we want to make our change and/or add unit tests. Refactoring means zero behavior changes--if the behavior changes you're not refactoring. We call them refactoring PRs and we keep them small and tightly focused.
Once we've got a nice clean place for our new code we branch off of the last refactoring branch and start creating the classes we'll need for our new functionality. That's often one class plus its unit tests per PR. The new classes aren't called by any existing code yet but they give the reviewer a chance to think about them, look at the tests, and see if there are any errors or missing test cases. We keep creating classes with unit tests and joining them with the other classes we've created in small steps until we've built everything we need for the feature.
Then the final PR joins the new feature to the existing system and activates it.
What else? The author usually discusses his design/code/PR plan with the reviewer at the beginning of the process. They agree on the design and the approach before a single branch is created. The reviewer is also reviewing the PRs as soon as possible after they are created so if things are going off the rails the author can get feedback from the reviewer before he wastes a bunch of time.
Note: this is what works for my team. We are a permanent team, we work together all time, and we value low defect, highly maintainable code. This approach might not work with the maintainer of an open source project you've never met or someone with other priorities for their code base and time.
Pointing to your last comment about "someone with other priorities", this seems to be the case with the project I'm contributing to. It hasn't had any development activity for a couple of years.
Truthfully, if the original author never merges my PR it won't bother me at all. But I felt obligated (regardless of the permissive MIT license) to contribute back to a project whose code I was basing my changes on.
If it turns out that the original author isn't very responsive, I'll probably just keep moving along with my fork since the changes I'm making are beneficial to the company I work for.
I'm a small business programmer. I love solving tough problems with Python and PHP. If you like what you're seeing, you should probably follow me here on dev.to and then checkout my blog.
Hey Jason, I recommend you follow the instruction/rules of the project. If the project doesn't have any rules, you might email the maintainer and ask what they prefer before you spend your valuable free time writing a PR that's going to get rejected after a 30 second review, which is probably what's going to happen to your PR.
In the absence of specific advice from the maintainer, I would follow my own advice in this post. PRs need to be easy to review. They need to be small enough that your brain doesn't give it before you understand the changes and make sure it's okay.
Since I wrote this post we've lowered the time limit for reviews to 40 minutes. My team rejects PRs when we hit the 40 minute mark--"too long" is the feedback the author gets. But that's our limit. The sweet spot seems to be around 20 minutes.
So how would we handle your situation? Chained PRs.
The first PR is just formatting the whole project in one commit with the message: "ran the project through the format -no manual code changes."
Then we'd branch off of that and create one or more stories to refactor the code where we want to make our change and/or add unit tests. Refactoring means zero behavior changes--if the behavior changes you're not refactoring. We call them refactoring PRs and we keep them small and tightly focused.
Once we've got a nice clean place for our new code we branch off of the last refactoring branch and start creating the classes we'll need for our new functionality. That's often one class plus its unit tests per PR. The new classes aren't called by any existing code yet but they give the reviewer a chance to think about them, look at the tests, and see if there are any errors or missing test cases. We keep creating classes with unit tests and joining them with the other classes we've created in small steps until we've built everything we need for the feature.
Then the final PR joins the new feature to the existing system and activates it.
What else? The author usually discusses his design/code/PR plan with the reviewer at the beginning of the process. They agree on the design and the approach before a single branch is created. The reviewer is also reviewing the PRs as soon as possible after they are created so if things are going off the rails the author can get feedback from the reviewer before he wastes a bunch of time.
Note: this is what works for my team. We are a permanent team, we work together all time, and we value low defect, highly maintainable code. This approach might not work with the maintainer of an open source project you've never met or someone with other priorities for their code base and time.
Pointing to your last comment about "someone with other priorities", this seems to be the case with the project I'm contributing to. It hasn't had any development activity for a couple of years.
Truthfully, if the original author never merges my PR it won't bother me at all. But I felt obligated (regardless of the permissive MIT license) to contribute back to a project whose code I was basing my changes on.
If it turns out that the original author isn't very responsive, I'll probably just keep moving along with my fork since the changes I'm making are beneficial to the company I work for.
That sounds like a very reasonable approach.
Cheers, Jason.