[Github-comments] [geany/geany] build.c: ASSIGNIF -> assign_cmd (#2256)

Nick Treleaven notifications at xxxxx
Tue Aug 13 18:37:46 UTC 2019


I'll edit the commit message before merging.

> Why even post a PR

So reviewers can check my changes don't introduce bugs.

> if you're reluctant to even minor suggestions?

I learnt that refactoring should focus on doing one thing well. Making multiple changes at once increases the risk of bugs, and makes it harder to review.

If I had written this code, then these review suggestions are fine and I would make the changes (I already thought of the intermediate variable and decided not to do it as explained above).

> why are you even doing this change at all if there are other refactoring of higher priority?

I felt like removing this macro. Removing this macro is more important than refactoring assign_cmd.

Also, I don't always have access to my development machine ATM, so this is a factor for me.


Sorry you feel you wasted your time. When reviewing code though, it's good to bear in mind the difference between new code and refactoring. Also suggesting small changes is actually adding work for a volunteer, consider whether the changes are good enough to accept without the suggestions implemented. Working on dlang, I often saw 'I suggest doing X, but it's not a blocker'. This keeps a good balance between submitter and reviewer. Let's be pragmatic.


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/2256#issuecomment-520956596
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20190813/11c5f445/attachment.html>


More information about the Github-comments mailing list