[Important] Updated Contribution Guidelines for Developers

Hello devs,

Over the last few days we have analyzed the reasons why version 8 release was not a smooth process and the core reason we have identified is

More lines of code per change = more chance of breaking and bugs

Also more lines of code are harder to evaluate, test and has more potential damage. So we are proposing a change in the way contributions are made to ERPNext.

We expect that you send pull-requests that change / add upto 30 lines of code excluding tests, templates and config files in one go. If you are working on a large feature, please break down your feature into small parts and get your contributions merged in parts.

Edit: We will go soft on the 30-lines limit as long as the contribution has test coverage, and enough screenshots and documentation.

We will now accept incomplete features as long as they are hidden and the path to the next contribution is clear. This means that if you are doing a feature, please add tasks in your GitHub issue and send a PR for each task.

This is a change in our current policy where we did not accept incomplete features.

Also we will avoid doing big releases from now on. Weekly releases with fixes will be on Tuesdays and Thursdays.

Feel free to add your feedback!

23 Likes

Great new Policy for contribution , more stable , less bugs :+1:

2 Likes

Good idea I think, easier to review, catch problems and avoid regressions.
Letā€™s see if always possible to break in about 30 lines, depending of features.

It will also help to merge PR quickly.
What about the merge frequency from develop branch into master ?

Because when sending PR to develop branch, it is easier to wait for getting back the master branch for production, than playing with git cherry-pick to deploy the PR.

Thanks

Iā€™ve seen other projects use a monthly ā€œfeature releaseā€ cadence with good success. Maybe ā€œhotfixā€ can be merged on Tuesday and Thursday, and ā€œdevelopā€ is merged once a month, maybe the second or third Thursday of a month.

1 Like

We will start implementing a new strategy for large pull requests:

We donā€™t want to have develop diverge a lot more and then put resources in testing it at the time of merge. The goal is to try a lean system (inspired by lean manufacturing). WIP is un-shipped code. Un-shipped code should be minimum.

If we are able to achieve that, we will get speed + cost + quality.

2 Likes

In this sequenceā€¦there is another think I donā€™t know if is already defined / documented. There are lots of github issues in erpnext repo that is about frappe (labeled framework) and others create directly in frappe.
Should I create issues on erpnext only or should (like how I actually do) create issues in frappe and erpnext differently?

2 Likes

I really appreciate the problem you are trying to solve. It is very likely to work well for small fixes which generally donā€™t result in adding/changing too many lines of code. But I donā€™t think 8.x issues are due to those kind of defect fixes.

The points you have mentioned in Cascading Pull Requests Ā· frappe/erpnext Wiki Ā· GitHub make perfect sense. In agile development, it is fundamental to breakdown the work into small testable chunks. However I am not quite sure everything can be broken down into 30 lines limit. If imposed strictly, it is likely to have adverse effect on the remote contributions, where turnaround time for pull requests are going to be significant compared to local contributions. Though small change are easy to review, it might be difficult to manage with big features. The overhead of context switch and frequent changes are very likely to overburden reviewers and gatekeepers defeating the very purpose.

Looking for the significance of number 30, I came across http://swreflections.blogspot.in/2012/12/rule-of-30-when-is-method-class-or.html. An alternate approach based on that would be to enforce 30 line rule for functions, classes and files rather than for pull request. In addition to that, we should also mandate automated tests where applicable. Those items coupled with a prior feature design document should make the code/feature much better to manage.

Keep the feature hidden from the user until it is complete

This is better done with framework support. One approach is to add ā€˜Devā€™ checkbox along with ā€˜Betaā€™, which makes the feature available only on ā€˜developmentā€™ mode. Perhaps there is more to do, but it is a good starting point.

2 Likes

We have to try different strategies. 30 is more a recommendation rather than a hard number. We donā€™t want to compromise either ā†’ speed or quality. So we have to see what works best.

1 Like

Lets just use erpnext issues, one place is easier to track.

agree that a more structured release cycle would be better for everybody.

a hotfix into develop twice a week is good, maybe once a month merge into master?

I would like to get confirmation about the branch usage, to make the merge process easier.

  • develop : for new features or non blocking fixes
  • hotfix : for blocking fixes like regressions, based on master branch
  • master : never send pull-requests against this production branch, reserved for merging hotfix and develop

Hotfix into master can be merged often (twice a week ?)
Develop into master can be merged slowly (once a month ?)

Am I right ?

Thanks

Later this week / early next week we will merge develop into master. Then here will be only develop + master. All PRs will go to develop.

2 Likes

Finaly!

Hi @rmehta

Is this still happening within the week?

Thanks!

A quick update here. I think we will go soft on the 30-lines limit as long as there is enough documentation, tests and coding standards are adhered to.

2 Likes

@rmehta Hi, I think the way of closing pull requests while waiting for informations or corrections is a bad habit.

Indeed, it is impossible to re-open it because it is not closed by ourselves.
Then we need to ask for opening it back, if you have no time and we donā€™t check to ask again, PR can stay closed and will never be merged.
6 days Iā€™m trying to re-open a PRā€¦
The process needs to be smooth to encourage contributions.

Thanks

1 Like