• magic_lobster_party@kbin.run
    link
    fedilink
    arrow-up
    141
    ·
    8 months ago

    Had a coworker who was a bit like this. They were tasked to do one simple thing. Required a few lines of code change at most.

    They end up refactoring the entire damn thing and introduced new bugs in the process.

        • Jesus_666@feddit.de
          link
          fedilink
          arrow-up
          26
          ·
          8 months ago

          Or, if the team does allow refactoring as part of an unrelated PR, have clean commits that allow me to review what you did in logical steps.

          If that’s not how you worked on the change than you either rewrite the history to make it look like you did or you’ll have to start over.

          • onlinepersona@programming.dev
            link
            fedilink
            English
            arrow-up
            7
            arrow-down
            4
            ·
            8 months ago

            Not with that attitude they won’t 😛

            Refactoring in PRs just makes it more difficult to review. “Do these lines belong to the goal nor not?”. Also, we’re human and miss things. Adding more text to review means the chance of missing something increases.
            Especially if the refactored code isn’t just refactored but modified, things are very easy to miss. Move an entire block of code from one file to another and make changes within = asking for trouble or a “LGTM” without any actual consideration. It makes code reviews more difficult, error-prone, and annoying.

            Code reviews aren’t there to just tick off a box. They are there to ensure what’s on the tin is actually in it and whether it was done well.

            CC BY-NC-SA 4.0

            • nick@campfyre.nickwebster.dev
              link
              fedilink
              arrow-up
              3
              ·
              8 months ago

              In my experience I haven’t had an issue because usually the refactorings are small. If they’re not I just hop on a call with the person who wrote the MR and ask them to walk me through it.

              In theory I’d like to have time to dedicate solely to code health, but that’s not quite the situation in basically any team I’ve been in.

              • onlinepersona@programming.dev
                link
                fedilink
                English
                arrow-up
                4
                arrow-down
                3
                ·
                8 months ago

                I haven’t had any trouble separating refactors PRs from ticket PRs. Make the ticket PR, make a refactor PR on that ticket PR, merge the ticket PR, rebase refactor PR on master, open ticket PR for review, done 🤷

                CC BY-NC-SA 4.0

          • BrianTheeBiscuiteer@lemmy.world
            link
            fedilink
            arrow-up
            1
            ·
            8 months ago

            I have a rule about this (not that I don’t break it at times). I only refactor in an unrelated story if it doesn’t delay deliverables and existing tests cover the code.

            And you’re generally right about tech that not being prioritized, but you should have a talk with your product manager/owner to strike a deal for some small percentage of your work to include tech debt. We were able to convince ours that it was otherwise affecting our velocity.

      • magic_lobster_party@kbin.run
        link
        fedilink
        arrow-up
        6
        ·
        8 months ago

        A tiny bit of value, but definitely not worth the pain and effort. It wasn’t exactly any technical debt that hindered our development.

        We had other places with way more pressing technical debt that could’ve been focused on instead.

      • GBU_28@lemm.ee
        link
        fedilink
        English
        arrow-up
        4
        ·
        8 months ago

        I hear you, but they should MVP the ticket, close it, then concisely collar the PM/lead and say “I know how to make this better and am hungry to do it. Let me address some tech debt next sprint. I got this and will keep it timeboxxed. I’ll also ensure my changes pass QA before coming to you”