Handling both cartographic and technical review

[pnorman]

There's been some discussion about handling the need to do both cartographic and technical review and the most efficient way to offer feedback when a PR needs both technical and cartographic changes. In some recent PRs it's felt like we were going in circles on technical matters when had never done a review on cartography and there was a reasonable chance the PR would never be accepted regardless of technical condition.

Obviously, technical errors bad enough to cause the style to fail to render need to be fixed before any review.

One suggestion was to do cartographic review before we worry about the technical details? On the other hand, if I'm travelling I often have free time when I can review code but not run the style.

[imagico]

The thing is technical issues are generally simple to communicate and almost always possible to fix - no matter if you agree with the complaint or not. This makes them easy to handle both for someone raising issues and for someone trying to address them. This is very different for cartographic/design related concerns.

So yes, i think an assessment of the overall aim of a PR and how it fits into the overall concept of the style should precede detailed technical review. Especially if technical concerns lead to significant work it is unfair towards the contributors if there is not even a preliminary decision on the desirability of the change.

[kocio-pl]

I'm sceptical about changing the way it is now.

Overall cartography rules are rather general (hence easy to check if given PR is following them), and I'm yet to see anything more specific, since it's so hard to reach agreement regarding additional rules. In my opinion the most important thing is the time to report all kind of problems (including cartographic review) and discuss them before merging, and I feel in most cases it's already long enough.

[sommerluk]

For me personally, the most important document was INSTALL.md. If I wouldn’t have figured out how to get the tool-chain working, probably I would have given up before even contacting somebody.

Probably it’s difficult to cover all the necessary information without forgetting anything.

However, I think the most important point is dialogue culture. The first contact is the most important one. I feel that my PRs have been discussed in a constructive way. There where suggestions that helped me to advance. If I had questions, I could ask and I got help. I feel this is a friendly community! (And #2206 might be in incentive to further improve this.) The real dialog culture is more important than any written document or Code of Conduct (#2289).

• It is good to see that also the maintainers discuss publicly, sometimes disagree about a topic and solve this in a constructive manner.
• For me it’s also important to see that @gravitystorm sometimes looks down a Github issue/discussion because of non-respectful behavior; this liberates the team from non-constructive, endless discussions and preserves a good social climate. (On the wiki, xxzme was banned too late; the social climate had suffered yet considerably. I welcome that openstreetmap-carto manages this sort of problems much more quickly.)
• It’s good that you can ask and get help if you do not know how to progress with a problem.

[kocio-pl]

I agree with @sommerluk that dialogue with contributors (and among maintainers) is the most important thing in review and it's hard to split it into specific parts, like cartographic or technical. We're in a process of updating design rules in CARTOGRAPHY.md, but while I think it's good and handy to clearly communicate our goals, it doesn't impact the way we should make the reviews IMO. I'm generally satisfied with the way it is now, I just like to avoid "freezing" review interaction for a long time with no decision.

@imagico Do you have some ideas how should it look like in practice?

[imagico]

I think we should aim for giving every PR that creates visible changes an early assessment regarding overall suitability and possibly with basic recommendations regarding the approach taken.

You have to be careful when doing that not to make a quick judgment from a premature first impression so it is highly advisable to be careful and restrained with such an early review. Such early assessment should not forestall more specific discussion of changes.

[Tomasz-W]

This ticket is more like a forum thread one, but the discussion is dead.

@kocio-pl I think it's useless at the moment. What about closing it?

[kocio-pl]

I think it works pretty well currently and we need some other things much more, like performance testing framework, but it already has its own ticket (#1287).

[matthijsmelissen]

I think we still start the technical review (and ask contributors for a series of changes) without deciding whether the proposed change makes sense cartographically.

[kocio-pl]

Would you like to change current practice in some way? Feel free to reopen the issue if you want to discuss it.

[jeisenbe]

This appears to still be an issue which I need to work on.

For example, PR #3464 is up to 97 comments, but we don't have clear consensus on rendering this feature.

It can be difficult to do a good cartographical review if the PR is a work in progress or if the code is not stable.