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?
This one might be hard to define for most people (and also include several of those listed before it).
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”?
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.
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.
+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:
I think we should add another item for “The change is compatible with a clustered XWiki installation” as we forget too often that clustering exists and information stored in-memory isn’t available on other cluster nodes.
+1 for the second option, I think we should ask the author of the PR to check/explain as much as possible, and the remaining parts will be handled by the reviewers.
+1
+1 for Usability, could be grouped with Accessibility.
For me that’s part of “Performance” but +1 to add it together under the same topic, and drop sustainability since right now we don’t even know what it means for us (apart from performance).
Yes the idea is for the developer to have in mind the topics, write what he/she can about them and have the reviewer make sure to remember the topics when reviewing.
Thanks
It sounds a lot like things I’d add in the ## Clarifications paragraph before.
Maybe we can use this as a more precise expectations for this paragraph. If we add a new paragraph, I doubt I’d have much to say in ## Clarifications in my PRs anymore.
__
Second small thought on the implementation in the template. We should make the list as default content (and not just a comment). This way it’s quicker to fill up because all of the category names are already here. If we leave it in comments, most people will probably copy/paste the list into an actual content block anyways.
IMO best is to go with checkboxes + space to provide comment.
So that people cannot forget, we have a clear view of what’s been handled or not and it’s easy to fill.
I have some more suggestions to add, though we could possibly also summarize them under other points:
I think both could be summarized under usability, and we don’t necessarily need to make them that verbose, but I would still explicitly mention both WYSIWYG editing and different screen resolutions.