Code review checklist

[John-Scott]

Is there any sort of code review checklist used internally by the core developers? Something that could be referenced by reviewers before approving changes, e.g. 'If this change modifies a public API call signature, be sure to clearly document the impact/follow the deprecation policy/etc'.

It's easy to quickly scan a pull request and give a :thumbsup: but it may not be immediately apparent to the reviewer that there are gotchas to consider. I accept that we'll always run into unanticipated issues (such is life!), but I'd like to see these 'lessons learned the hard way' turned into a formal checklist so reviewers will be prompted to look for these types of issues going forward.

Concretely, I've been bitten several times over the years by backwards incompatible changes that were not clearly identified in the release notes/history/changelog of django-cms and related plugins. 

A recent example is described in this djangocms-link issue and pull request. In this case, the default template changed paths and the context variables also changed. However, the changelog only said "* Cleaned up file structure". It wasn't clear at all from this that this impacted those of us who had customized the default template. Users are left to either experience the bug in production after what was assumed to be an innocent upgrade, or they have to be extremely thorough and review every git commit before upgrading.

A checklist or some such document could be helpful in bringing structure to how future changes are evaluated.

Thoughts?

[Angelo Dini]

Hello @John

We do have some sort of checklist though it's mostly shared verbally between the core developers.

I think it would make sense to eventually further expand http://docs.django-cms.org/en/release-3.4.x/contributing/code.html

any other opinion?

Cheers

Angelo

[John-Scott]

I think it would be great to properly document this knowledge in writing!

The checklists will probably be unique for django-cms vs the various plugins hosted under https://github.com/divio/* since they each have their own unique gotchas and architectural concerns.

I'm just hoping to formalize the process so that everyone has a handy reference when reviewing pull requests which might help them catch issues before they are merged into a release. I'm not a core developer, so I defer to others on the specifics of how this is accomplished.

Thanks!