Reduce merge conflicts

[davidskalinder]

Well @sinanpl as you can probably see I've been doing a flurry of work on this in the past couple weeks, and there's getting to be a fair queue of PRs lined up for review. Having spent most of the last day just figuring out how to merge them all into my testing branch, I can attest (though admittedly my vimdiff-fu isn't the sharpest) that there's getting to be some gnarly merge conflicts between the PRs as they all get farther away from the upstream master branch that I pull to start each new feature. (Most of the merge conflicts are in fact straightforward despite some nasty diffs; there were a few I found that actually required new code though, so I tried to leave comments for those in in the PR threads.)

Of course one solution is to have a quicker turnaround time for getting PRs accepted, but I know that's not easy. So I'm wondering if there's anything I can do to help reduce the merge conflicts? Some possibilities I can think of are:

• I could be less careful about separating features (e.g., the three stages of refactoring of the bootstrapping function, which I could have rolled into one, not to mention the two other PRs that depend on those). (Downside: harder to accept some features and not others.)
• I could also be less careful about always trying to build from a clean upstream branch, building more features on top of others. (Downside: harder to accept a new feature while rejecting the dependencies.)
• I could start maintaining my own master branch that's up to date with features that I think are ready to go and we could periodically do an omnibus PR upstream. (Downside: the forks could drift away from each other, and that big PR would probably have to either be accepted on faith or be a huge mess.)
• You could give me push permissions on this repo, or maybe just to a development branch or some such (if permissions can be assigned by branch?). Then I could merge changes to that branch and once they're stable that branch could be merged to master. (Downside: probably not much better than me just maintaining a branch in my own repo... In fact, maybe worse, since we probably can't use GitHub's PR review functionality on the merge?)

Ultimately, I think all of these amount to making PRs less atomic and spending less time reviewing each others' contributions, and I know we discussed before (and I agree!) that smaller PRs with close review would be better. But I think the tradeoff is always going to be that more atomic PRs will make for tough merges unless they happen fairly quickly? Which ultimately @sinanpl is going to mean more work for you as the maintainer to get them accepted before the next round of development.

As far as my workflow goes, I'm hoping that I can move on to analysis work next week and not have to crack open this codebase for a while, but on the other hand it's possible that I'll find another thing to build and come up with another batch of PRs! So like I say @sinanpl let me know what you think -- it's very possible that I'm missing a good workflow trick or two that would help make all this easier.

[sinanpl]

Hi @davidskalinder I didn't get to much coding/review time lately, so I am inclined to accept your proposal.

• I have sent an invite to make you a collaborator
• I have created a development branch. If there is something you want to check before pushing it to master, please use this as a step in between. Especially when you foresee multiple PRs that might conflict
• I've set-up a Github Action workflow for the styling. After any push to any branch, this will restyle the package and recommit with styling. I believe this should take away a bit of the burdens you mentioned in other issues. Might be useful to merge dev into your feature branch to resolve the styling part or reopen the PR with the same branch to trigger the styling. If everything goes right, in case of styling is needed, a new commit will appear. Check the last three commits of this branch for example: https://github.com/sinanpl/OaxacaBlinder/commits/test_styling/
• I also tried to create workflow to test the package as well, but have to revert is due to failing examples. In progress.

Feel free to close some PRs by yourself (maybe first into dev, then master?) and issues as well.

[davidskalinder]

Okay looks good, many thanks! Comments/questions:

Hi @davidskalinder I didn't get to much coding/review time lately, so I am inclined to accept your proposal.

Yeah I fully realize this is mostly a matter of balancing what we want to do versus how much time we each have available. I know at the moment you've got limited resources for this project, so I really appreciate all you have been able to do!

• I have sent an invite to make you a collaborator

Should be all set, thanks!

• I have created a development branch. If there is something you want to check before pushing it to master, please use this as a step in between. Especially when you foresee multiple PRs that might conflict

Okay, sounds good. For complex batches of merges I might even create a staging branch to make sure the merges work and then merge that into the development branch.

• I've set-up a Github Action workflow for the styling. After any push to any branch, this will restyle the package and recommit with styling. I believe this should take away a bit of the burdens you mentioned in other issues.

That sounds terrific, thanks! I haven't used a setup like that before, but hopefully it'll all just work automagically...

Might be useful to merge dev into your feature branch to resolve the styling part or reopen the PR with the same branch to trigger the styling.

Hmm yes you might be right. I think I should be able to just merge the dev branch into each PR'd branch and push those branches back, since that should update the PRs automatically?

Feel free to close some PRs by yourself (maybe first into dev, then master?) and issues as well.

Okay, great! Hmm and now I need to think about what my workflow should be for this, heh... I think I will try to get most of them into the dev branch and then keep that installed (perhaps with a private workaround for #18 while that's pending) for my analysis project so that I can test the changes? I'll give that a bash and see how it goes anyway.

[davidskalinder]

Hmm yes you might be right. I think I should be able to just merge the dev branch into each PR'd branch and push those branches back, since that should update the PRs automatically?

Hmph, it seems that for some reason the styler action isn't triggering when the only change is merging in the dev branch. (Specifically, the case that doesn't work is: take a branch that is up to date with the remote, merge my local upstream_dev into it with a new commit for the merge, then push.)

Other new commits do seem to trigger the action though, so I could just add a bad line break or something and push that. Or I suppose I could just run styler::style_pkg() locally. That seems the best, so I'll give that a try.

Meanwhile, I'm realizing that part of the reason for the tricky merges is that I didn't notice the reformatting that happened at 6474170568, which was after I pulled the upstream branch for some of my features. So just merging an updated upstream into the features with that in mind has clarified things a bit, in the individual features at least -- hopefully that will be the case when I merge them as well.

[sinanpl]

Actually, the trigger is on each push to any branch. If there are no additional commits to your feature branch, no styling will be applied.

For the dev / master branch I only expect styling after the first merge commit (after styling was set up). This is a guess though, not tested.

Does the above make sense? Let me know if you need more support on how to deal with this.

[sinanpl]

@davidskalinder some notes for clarity.

the updates I had implemented only work when you have the workflow file in your branch as well. So you should first merge master into your feature branches.

In addition, I updated to workflow to enable manually triggered runs. So now you can navigate to Actions and trigger the styling worklow for a certain branch. The results can be monitored such as here.

The only condition is that the workflow yml files are in your branch you want to style.

So, I'd propose you

• merge dev into every feature branche you intend to merge
• run the styling on the feature branches manually if you will not commit R files
• run the workflow on the master (if you do this earlier, you'll need to solve merge conflicts adding the workflow file to your branches)
• complete the PRs

Hope this helps

[davidskalinder]

• if you will not commit R files

Ah, I think this is the key -- the merge commits (at least some of them -- maybe just the ones where there were no merge conflicts?) probably didn't commit any R files (or .Rmd or whatever else is in the list).

So the above looks great -- for the branches that don't trigger the action I can either run the action manually on GitHub or just do styler::style_pkg() locally (probably? I'll test to make sure they come out the same).

Thanks again!

[davidskalinder]

I can either run the action manually on GitHub or just do styler::style_pkg() locally (probably? I'll test to make sure they come out the same).

Okay it took some figuring out to get the manual triggers to run on my fork, but yes, they do come out the same. That being the case, I'll probably just do the styling locally after merging the new dev branch into each feature branch.

[davidskalinder]

Okay, I just accepted all of my outstanding PRs into the dev branch, cleaning up merge conflicts along the way. It was not the tidiest process, and there was probably lots of unnecessary restyling along the way, so unfortunately those diffs might be messy if we ever have to revisit them.

But all 19 of the tests added by the PRs are now passing in the dev branch (so @sinanpl, thanks a million for insisting on tests -- this would have been a disaster without them); so I'm reasonably confident that everything has worked. I'll pull that dev branch for my substantive project next week, so hopefully if anything turns up amiss I'll spot it relatively quickly.

I'll close this issue for now (in the hopes that I won't need to reopen it again in a few days). @sinanpl, thanks again for your help getting this stuff sorted!

[sinanpl]

Awesome, great work @davidskalinder, nice progress. We can soon merge dev into master. I found something for which I'll open a new issue.