Skip to content
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

add ColPrac guide #1348

Merged
merged 1 commit into from
Jul 15, 2021
Merged

add ColPrac guide #1348

merged 1 commit into from
Jul 15, 2021

Conversation

willtebbutt
Copy link
Member

@willtebbutt willtebbutt commented Jul 3, 2020

Adds the new collaborative practices guide. This seems to have buy in from several members of the team, but would require everyone to be on board.

We already do pretty much everything, the only thing that we don't do is release a patch version after every merged PR. The reason to like this is that

  1. it makes identifying where a bug was introduced really easy (if we accidentally introduce a bug with a release)
  2. it means that patches get to users instantly, rather than having to wait for minor releases that we only do every so often

It doesn't really have any downsides because there's plenty of tooling in place to handle everything.

@yebai @xukai92 @devmotion @cpfiffer and anyone else that I've missed.

@willtebbutt willtebbutt requested a review from yebai July 3, 2020 15:04
@codecov
Copy link

codecov bot commented Jul 3, 2020

Codecov Report

Merging #1348 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1348   +/-   ##
=======================================
  Coverage   69.67%   69.67%           
=======================================
  Files          27       27           
  Lines        1540     1540           
=======================================
  Hits         1073     1073           
  Misses        467      467           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6db5962...56ddce3. Read the comment docs.

@devmotion
Copy link
Member

We already do pretty much everything, the only thing that we don't do is release a patch version after every merged PR.

I think that would help a lot since currently we accumulate quite many changes in some upstream packages which then "suddenly" require fixes in, e.g., Turing when they are released, although some of them could have been integrated much earlier and weren't breaking. I'm wondering if these guidelines indicate that one should not work with a separate dev branch (apart from maybe if one wants to split work on some new features that belong together in separate PRs?)?

@willtebbutt
Copy link
Member Author

@yebai just bumping this. I agree with @devmotion that currently Turing tends to accumulate quite a few non-breaking changes between releases, which is bad because bug fixes / new but non-breaking take longer to make it to users.

If it is the case that you want to be able to make multiple breaking changes between releases, it might make sense to move to a (very slightly) more sophisticated set up with git whereby

  • master contains the most recent release at any point in time. Any non-breaking changes (bug fixes etc) get merged directly into master and a new patch version is released immediately
  • a separate dev branch contains all breaking changes, and is merged into master when a minor version release happens.

For example, suppose we're currently on version 0.13.5.

  • if someone produces a bug fix, it gets merged directly into master and bumps the version to 13.6. This change is also merged dev so that it remains up-to-date with master
  • if someone is working on a new feature that isn't breaking (performance-related, fancy new syntax that is backwards-compatible etc), the same happens
  • new breaking changes get merged into dev until a release is ready to go, at which point dev is merged into master and version 0.14 is released.

@cpfiffer
Copy link
Member

cpfiffer commented Aug 3, 2020

I think one of the issues I've noticed with the dev workflow is making sure the minor changes that go directly into master are reflected in some way in the dev branch -- does anyone know a way to automate that kind of thing?

@xukai92
Copy link
Member

xukai92 commented Aug 6, 2020

@willtebbutt's proposal sounds great.

I think one of the issues I've noticed with the dev workflow is making sure the minor changes that go directly into master are reflected in some way in the dev branch -- does anyone know a way to automate that kind of thing?

I think we will have to choose which branch (master or dev) to merge when crating the PR.
Automatically making PRs that go in to master also go to dev should be possible via GitHub Actions but I don't know exactly how.

@yebai
Copy link
Member

yebai commented Aug 24, 2020

This looks like a good plan. It would be nice to have an automatic mechanism for keeping dev and master branches synced for bug-fixing PRs. For example, if a (non-dev) PR is directly merged into master, then automatically open a PR for the dev branch.

EDIT: Kai’s proposal is similar, but it would be helpful to go through a review process for automatic PRs against the dev branch.

@devmotion
Copy link
Member

It would be nice to have an automatic mechanism for keeping dev and master branches synced for bug-fixing PRs. For example, if a (non-dev) PR is directly merged into master, then automatically open a PR for the dev branch.

Something like https://github.com/TuringLang/DynamicPPL.jl/blob/master/.github/workflows/pullrequest.yml could maybe work. At least just opened the first PR to the dev branch.

@yebai yebai merged commit d029198 into master Jul 15, 2021
@delete-merged-branch delete-merged-branch bot deleted the wct/col-prac branch July 15, 2021 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants