[gruez]

>- The focus of reviews is not on correctness but stuff like naming, docstrings, and design.

I agree that focusing on naming/docstrings doesn't make much sense, but design feels like it's fair game. What type of "design" stuff is showing up in your reviews?

[krimbus]

Indeed! Design suggestions are fair game, but they are quite rare nowadays and their quality has been dwindling.

The last one I got felt too arbitrary: I decided to improve the behavior of an unused feature from one of our core libraries to unblock a colleague working in the application. The change was straightforward and it took me 1hr to get it into a PR. The feedback was that we should keep the old behavior available, just in case. So I spent a full afternoon wrangling C++ templates to fit in a new API that offered both the improved and old behavior for which there is no requirement.

What started as a small detour from my regular work to help a fellow engineer ended up costing me a day.

In the end, I told my colleague that he would have to take over the PR as I had lost too much time on it. Sure enough, he had to change all parameter names cause the types already describe them well enough: `foo(Timeout connection_timeout)` -> `foo(Timeout connection)`.

The resulting code looks great, but now it is clear that I have a more pragmatic style. I recognize that I simply failed to stand my ground, and I'll take that into account.

[tharkun__]

I read all of the comments here with much interest. Thanks for putting in this specific example.

I would love to have a chat with your architect. If you (or he) gave me a PR with

    foo(Timeout connection)

I would ask him to improve the naming. That thing is not a connection. In the places this variable will be used it makes no sense to use a connection. Take this example usage, potentially way down in the implementation of foo. Let's say an imaginary send function that takes the data and a timeout).

    send(data, connection)

If I read this code I will think that it takes data and a connection. But it doesn't. It takes data and a timeout. I have to know or look up the type of connection to understand what is happening.

Instead your original

    foo(Timeout connection_timeout)

Will make absolute perfect sense without knowing or having to look up the type. The following code can be read without having to stop and cross reference information. What you see is what you get. Less cognitive load, which is awesome. You put in thought to make awesome readable code once and every subsequent reader for all eternity can enjoy being able to just read and immediately understand what your code does and what everything is.

    send(data, connection_timeout)