-
Notifications
You must be signed in to change notification settings - Fork 25
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
Pull Requests Review. please #113
Comments
Hi @Levitanus, I totally get your frustration about PR not being reviewed. We don't shout in here though, so please avoid hammering the exclamation mark key in the future. Most of the pull requests you've submitted that haven't been merged yet, I've already started reviewing them. However I've felt confused because they sometimes encompass different things. Bug fixes, API additions and general API design decisions can be melted together in the same PR. All changes have to be reviewed independently, because some of them are acceptable the way they are and some not. I've had some trouble figuring out a good way to discuss them all at once and eventually have given up. I guess in several cases, you have grouped things together because they somehow belonged to the same logic and depended on one another. I think this fact should be interpreted as a sign that there's an underlying important API decision. Thus it should probably first be discussed to decide on the right design before writing code that may not match with the final specification. That being said, you're definitely making a good point that all this must be cleaned up. I'll try to tackle all of them one by one. However in the future please try to create one pull request per additional feature, or create an issue to discuss big changes in advance (as in #59 or #61). It would greatly help things move faster. |
Sorry for exclamation. But I really didn't know how to start discussion... The last weeks I've tried to clear things as much as possible. But the best way to clear them more — move from the earliest to the last, as we can rebase them to master in progress. Actually, Hotfixes (#92) remained just fixes while I didn't feel It's more clear to add some obvious methods that should not need a discussion. And I still try to keep this branch as straight as possible. The other decisions have more points to talk about, so... I'm ready to do this)) Maybe #114 is the one which is monolith solution, occasionally, passed the test for stability: longer than half of the year it still parses Julian repo correctly and works on the base of the last stable reapy release (0.9). |
I understand You don't have enough time and strength to get them into work, but my GitFlow CoungFu is already defeated by the amount of changes waiting for approval.
As You can see, #111 already has only a couple of commits that need discussion, and the rest is just a copy of #92, on which the #111 is based on.
Now I want to extend Markers and Regions by MarkerList and RegionList classes, with
ReapyObject.map
method usage in mind (which is introduced in #81). So, I cannot put these changes inside #81 as I still need features from #111, but I can't post them into #111 as the changes are logically based on #81.One by one each pull request is not a real matter, but they all are at least in my projects for a half of the year and, despite the fact I really depend on them yet (and I will suffer a lot if API changes), now the adding of new reasonable functionality is restricted by cross-dependencies of one PR to other.
Also, we already have some duplicates of pulls solved the same issue by different ways and some issues about things introduced or fixed more than a half year ago...
The text was updated successfully, but these errors were encountered: