Ask HN: How to deal with well-intentioned PR comments?
7 comments
The hope here is that this new team member is going to stick around for a while and contribute new insight to the team. Since he's senior I hope you hired him so you could learn from him.
You might want to have a meeting with this new person, his senior peers on the team, and a manager who can oversee and referee things. Talk with him about the stuff he wants to change. If everyone agrees that his ideas are good then you should talk about how to incorporate his ideas into the code.
As a senior person he should understand that simply parachuting in new style to existing code can cause problems. On the other hand you should be willing to find places where these improvements can be added appropriately, perhaps in new code or bulk refactoring that would happen anyway. You want him to feel that you are on his side so this new style should be the team's new style, enforced by everyone where appropriate, and not just his thing. If you review a PR for new code, you'll want to make sure it conforms to the new style, and say it like "following our team's style for new code, class X should be split in two because of reason Z" and not "please split class X in two because that's what NewGuy wants, you know how he can be."
If you just say no to his ideas even if you agree they are good then he might become unhappy and, if rejecting his ideas becomes a pattern, might end up leaving.
You might want to have a meeting with this new person, his senior peers on the team, and a manager who can oversee and referee things. Talk with him about the stuff he wants to change. If everyone agrees that his ideas are good then you should talk about how to incorporate his ideas into the code.
As a senior person he should understand that simply parachuting in new style to existing code can cause problems. On the other hand you should be willing to find places where these improvements can be added appropriately, perhaps in new code or bulk refactoring that would happen anyway. You want him to feel that you are on his side so this new style should be the team's new style, enforced by everyone where appropriate, and not just his thing. If you review a PR for new code, you'll want to make sure it conforms to the new style, and say it like "following our team's style for new code, class X should be split in two because of reason Z" and not "please split class X in two because that's what NewGuy wants, you know how he can be."
If you just say no to his ideas even if you agree they are good then he might become unhappy and, if rejecting his ideas becomes a pattern, might end up leaving.
The way you are using the pronouns "we", "he", "our", and "I" in your text could hint at an underlying personal issue. I.e. you don't like this guy and perhaps feel threatened by him. "we know that", "we are certainly open for suggestions", "we think"... Is that a royal we or do you really know what everyone else in your team thinks? :) Also, unless you own the company, it is not "our codebase", it is "the company's codebase". Also, the answer to your question is to tell the guy to go ahead and commit his suggestions.
its certainly his use of pronouns that is the problem.
Tell him we don't have refactoring in the timeline currently so we need to just stick with what we have setup currently.
But ask him to put those thoughts/comments in a shared refactor document then the team can discuss and prioritize and add groups of refactoring tasks in to the timeline in the future.
But ask him to put those thoughts/comments in a shared refactor document then the team can discuss and prioritize and add groups of refactoring tasks in to the timeline in the future.
so explain what you write here and don't follow all of his suggestions? I'm not sure why this is a question?
Why is he suggesting the changes? Because they are good practice or because they want to make an impact?
When dealing with conflicts understanding the motivation of both sides is key.
Oof the junior-senior. A classic in coding. I’d just ignore their comments and get someone else to PR.
Now, our new team member suggests in PRs things like the following:
- "let's rename that class to X so that it includes the suffix Y because of reason Z". This suggestion makes totally sense... but in our codebase our classes have already a certain naming convention that doesn't go the way our new team member thinks is best.
- "following SOLID principles, class X should be splitted in two because of reason Z". Again, the suggestion makes totally sense, but if we follow it, it would make class X a snowflake in our codebase.
The worst thing is that our codebase is not in a bad shape, so his suggestions, while definitely well-intentioned and correct in most software engineering books, lack context and would make our codebase not necessarily better. There is, of course, the possibility to refactor our entire codebase to follow the best practices out there, but we think that will give us little to no benefit (also, it will take time and it will be hard to argument this kind of decision to management because, as I said above, it's "working fine")