Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

Large PRs #576

Closed
jpfairbanks opened this issue Mar 29, 2017 · 3 comments
Closed

Large PRs #576

jpfairbanks opened this issue Mar 29, 2017 · 3 comments
Labels
discussion requires in-depth discussion - participation encouraged

Comments

@jpfairbanks
Copy link
Contributor

We had the huge abstraction PR (#541) that was a lot of work. This is a place for people to comment on how to avoid such giant PRs in the future.

The PR grew because there were a few changes that needed to happen at the same time.

  • Abstraction of the interface
    This was always going to take a lot of lines but most of the changes would have been simple changes to the type constraints on function signatures.

  • JLv0.6 compatibility
    This would have some complex changes for things like channels, constructor syntax.
    It would also require waiting for PRs on dependencies.

  • Testing and Documentation Improvements.
    These are mostly additions and thus easy to approve.
    The switch to test sets meant that a lot of lines would get touched but only whitespace would be affected.

  • Benchmarking
    Introducing benchmarks was also mostly additional lines.

Conclusion

I think the changes that were introduced in the big PR were actually 4 separate PRs put together.
The problem is that they needed to be scheduled to reduce conflicts. Once we had an open PR that diverged far enough from master there was no choice but to lump them all together. So we needed to do 0.6 compatibility; then abstraction; and then benchmarking, testing, and documenting in parallel. Each PR would be squashed before merging and each commit on master would be a working version of the code.

@jpfairbanks jpfairbanks added the discussion requires in-depth discussion - participation encouraged label Mar 29, 2017
@sbromberger
Copy link
Owner

One issue that I ran into is that it made sense to update the docs as I went through the abstraction. There were tons of errors in the docs, and we changed a bunch of stuff with traits, and it all needed notes.

I suppose I could've kept two copies of the repo - one for abstraction work, and one for docs - but that seems like a recipe for merge conflict disaster.

@sbromberger
Copy link
Owner

Benchmarks were actually introduced in #538, so any changes to benchmark/ were to support 0.6 or abstractions.

@sbromberger
Copy link
Owner

I think this is just a conversation between you and me at this point, @jpfairbanks , so I'll close it out but feel free to reopen/comment/whatever.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion requires in-depth discussion - participation encouraged
Projects
None yet
Development

No branches or pull requests

2 participants