Make sure PR writers have thought about transverse topics

Development
1

Hi,

I’d like to propose amending the PR template to make sure transversal topics have been thought of, with something like:

Please make sure you have considered and addressed the following aspects in your code:
* Security
* Performance
* Accessibility
* Backward Compatibility
* Automated tests and quality in general (SonarQube checks)
* Localization
* Sustainability impact

WDYT?

1 Like
2

This one might be hard to define for most people (and also include several of those listed before it).

3

What I’m wondering if we shouldn’t force the developer to write a few words regarding each of these topics in order to make it easier for reviewers to understand what the change is about. For example, I could imagine that there might be a change that is necessary for security purposes but negatively affects the performance (like access right checks on the tag cloud). Alternatively, we could also formulate these points a bit more like a checklist such that in the best case, the developer can simply check the boxes. For example, instead of “localization”, we could write: “All user-visible strings are localized and localization keys and messages follow our best practices”. The expectation here would be that the developer either checks the checkbox, or adds an explanation why it is necessary to make things worse in some way.

In the list above, I’m missing a general “Usability” and maybe also “UI consistency” point.

What about adding “Resource usage”/“The resource usage of XWiki isn’t (significantly) increased” instead of “Sustainability impact”?

4

I’d be -1 on that idea: it’s already not easy to contribute to XWiki, and I’ve the feeling it would be adding another blocker to create a PR for a possible contributor.

However, I think that option is quite interesting: it points out to the contributor what they should do / should check.

5

We could make it only a requirement for some developers. Or, to make it optional to create a PR, but mandatory to merge it, making it mandatory for the reviewer to think of those aspects before merging.

6

+1 to the idea of updating the PR template

+1 to a more precise wording with links to best practices where possible.

+1 we don’t have a lot of new code contributors right now so it would in practice be almost the same.

To get a correctly defined rule, we could reuse some kind of group we already have on Github and make it mandatory to fill these for people opening PRs from these group but not for others: