• magic_lobster_party@kbin.run
    142·
    9 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
          25·
          9 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.devEnglish
            74·
            9 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
              3·
              9 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.devEnglish
                33·
                9 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
            1·
            9 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
        6·
        9 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.eeEnglish
        4·
        9 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”

    • kamen@lemmy.world
      351·
      9 months ago

      The pipeline should handle formatting. No matter how you screw it up, once you commit, it gets formatted to an agreed upon standard.

        • FizzyOrange@programming.dev
          7·
          9 months ago

          Yeah I think that’s what he meant. You don’t want CI editing commits.

          I use pre-commit for this. It’s pretty decent. The major flaws I’ve found with it:

          • Each linter has to be in its own repo (for most linter types). So it’s not really usable for project-specific lints.

          • Doesn’t really work with e.g. pyright or pylint unless you use no third party dependencies because you need a venv set up with your dependencies installed and pre-commit (fairly reasonably) doesn’t take care of that.

          Overall it’s good, with some flaws, but there’s nothing better available so you should definitely use it.

          • zalgotext@sh.itjust.works
            1·
            9 months ago

            I’ve used pre-commit pretty extensively over the years and I’m confused.

            Each linter has to be in its own repo (for most linter types). So it’s not really usable for project-specific lints.

            Not sure what you mean by this. I have pre-commit set up to do linting in several different projects, and even have it running multiple differently-configured lint jobs in the same repo.

            Doesn’t really work with e.g. pyright or pylint unless you use no third party dependencies because you need a venv set up with your dependencies installed and pre-commit (fairly reasonably) doesn’t take care of that.

            Again, I have pre-commit set up on multiple repos running pylint with multiple different plugins. Pre-commit absolutely does take care of setting up venvs with needed dependencies.

            • FizzyOrange@programming.dev
              1·
              9 months ago

              Not sure what you mean by this. I have pre-commit set up to do linting in several different projects, and even have it running multiple differently-configured lint jobs in the same repo.

              I don’t mean using lints, I mean writing custom ones. Say you have a custom lint you want to use but it only will ever be used for that specific project. You can’t just put the lint code in a subdirectory. It has to go in a separate repo.

              Pre-commit absolutely does take care of setting up venvs with needed dependencies.

              Again I think you might be misunderstanding. It will install pylint fine, but if your project does e.g. import yaml, it’s not going to set up a venv and install pyyaml for you.

              • zalgotext@sh.itjust.works
                1·
                9 months ago

                Say you have a custom lint you want to use but it only will ever be used for that specific project. You can’t just put the lint code in a subdirectory. It has to go in a separate repo.

                You can run locally defined hooks with pre-commit, just define them in the repo: local section of the .pre-commit-config.yaml, and have it run a bash/python/whatever script or something that invokes your custom linting, wherever it lives in your file structure.

                It will install pylint fine, but if your project does e.g. import yaml, it’s not going to set up a venv and install pyyaml for you.

                Yeah I misspoke/misremembered there. For Python based stuff, it uses the currently active virtualenv or your global python install, so it relies on you installing your own dependencies. Which isn’t really that big a deal imo, because you need to install those dependencies to run/debug/test locally anyways.

                • FizzyOrange@programming.dev
                  1·
                  9 months ago

                  You can run locally defined hooks with pre-commit, just define them in the repo: local section of the .pre-commit-config.yaml

                  Sounds like you’re just googling it rather than actually speaking from experience. Suppose I have written a Python lint and it’s in my ci/lints/foo folder. How do I tell pre-commit that? (Hint: you can’t)

                  Which isn’t really that big a deal imo

                  For small Python projects, maybe not. The project I’m working on has multiple sub-projects and those each have their own venvs, pyproject.tomls, etc.

      • sunbytes@lemmy.world
        3·
        9 months ago

        Some diff tools don’t handle indentation by default.

        So if you add a wrapper, it counts everything inside it as “changed”

        • kamen@lemmy.worldEnglish
          2·
          9 months ago

          Pre-commit hooks is a common approach to this, so that whatever is committed gets processed. Another possibility would be to set a bot on the repo to do automated commits after human-made ones, but that can get a little noisy.

  • AnarchoSnowPlow@midwest.social
    26·
    9 months ago

    Last time somebody did this to me there were a lot of sit downs about how to properly chop up large scale code changes and why we don’t sit on our own branch for two months.

    “How long will this take to get in?”

    “Well, two weeks for me to initially review it, a week for you to address all the changes, then another week or so for me to re-review it… Then of course we have to merge in all the changes that have been happening in primary…”

    • BrianTheeBiscuiteer@lemmy.world
      3·
      9 months ago

      Last time I got this PR I was like, “Okay, I’ll do my best, but you asked the guy that has like 30 mins a day to actually focus and look at someone else’s code AND yours isn’t the only PR I’ll have to look at this sprint. Have fun reminding me about this for the next week.”

  • swordsmanluke@programming.dev
    14·
    9 months ago

    Net removal of 1500 LoC…

    I’m gonna make you break this up into multiple PRs before reviewing, but honestly, if your refactoring reduced the surface area by 20% I’m a happy man.

  • JATtho@sopuli.xyz
    131·
    9 months ago

    Please, no, I get flashbacks from my 6-month journey (still ongoing…) of the code review process I caused/did. Keeping PR scope contained and small is hard.

    From this experience, I wish GitLab had a “Draft of Draft” to tell the reviewer what the quality of the pushed code is at: “NAK”, “It maybe compiles”, “The logic is broken” and “Missing 50% of the code”, “This should be split into N PRs”. This would allow openly co-develop, discuss, and steer the design, before moving to nitpicking on the naming, formatting, and/or documentation details of the code, which is likely to drastically change. Drafts do work for this, but the discussions can get uncomfortably long and convolute the actual finishing of the review process.

    Once both reviewer(s) and the author agree on the code design, the “DraftDraft” could be collapsed into a link in an normal Draft to be mocked next. The scope of such draft would be limited by the earlier “DraftDraft”.

  • tatterdemalion@programming.dev
    122·
    9 months ago

    This does seem like a potential issue if the PR is itself implementing more than one vertical slice of a feature. Then it could have been smaller and there might be wasted effort.

    If the patches are small and well-organized then this isn’t necessarily a bad thing. It will take more than one day to review it, but it clearly took much more time to write it.

    • Throwaway@lemm.ee
      5·
      9 months ago

      True, but at the same time its very possible to go too small. A bunch of one line code reviews can really slow progress easily.

      • magic_lobster_party@kbin.run
        5·
        9 months ago

        Stuffing multiple tasks into one PR is often bad.

        • It’s harder to review. As a reviewer it’s difficult to know which code change is related to which task.
        • It’s harder to verify. Did you really test every change you made?
        • You might end up with a “hostage” situation. There might be a few code changes in the PR that looks good and is really wanted, but other code changes in the same PR of lower quality. As a reviewer, should you just let these lower quality code changes slide so you can bring in the code change you really want? Probably not, but you’re going to let it slide either way.
      • tatterdemalion@programming.dev
        3·
        9 months ago

        Right but it’s pretty rare that a tiny PR actually accomplishes a valuable user story.

        So my point is just that lines of code is mostly irrelevant as long as it’s organized well and does no more than necessary to accomplish the agreed upon goal.

  • dan@upvote.au
    8·
    9 months ago

    I try to keep my changes under 300-350 lines. Seems like a good threshold.

    I’m still annoyed that Github doesn’t have good support for stacked diffs. It’s still not possible to say that one PR depends on a different one, and still has no ability to review and land them as a stack.

    • iknowitwheniseeit@lemmynsfw.com
      6·
      9 months ago

      How is this different from creating a feature branch and making your PR against them until everything is done, then merging that into the main branch?

      • nomen_dubium@startrek.website
        1·
        9 months ago

        also iirc gitlab does offer something like this as a feature now with “merge trains” (though i’ve never really used it, usualy just go for the feature branch out of habit x) )

      • dan@upvote.au
        1·
        9 months ago

        Making PRs against a feature branch still has the same problem.

        Say you’re working on a major new feature, like adding a new log in flow. It’s a good idea to split it into many small changes: create initial log in form, implement user sessions, add SSO support, add “remember me” functionality, etc.

        Those changes will likely depend on each other, for example adding the “remember me” checkbox requires the form to actually be built, and you probably don’t want to wait for the reviewers to review one change before continuing your work. This means you naturally end up with PRs that depend on each other in a chain.

        Stacked PRs (or stacked diffs, stacked MRs, whatever your company calls it) means that:

        1. The code review tool lets you specify dependencies between the PRs, for example the “remember me” PR depends on the initial login form implementation
        2. It shows the dependencies visually in the UI, like a chain or tree
        3. Changes can’t be landed until the PRs they depend on have been reviewed
        4. There’s a way to land an entire stack
        5. When you review a PR later in the stack, it doesn’t show any of the changes that were made earlier in the stack. Each PR focuses just on the changes in that part.
        6. For each PR, the build steps and linters run for all the changes in the stack up until that point. It helps detect if incompatible changes are made earlier in the stack.

        Making all your commits directly to a branch then creating a PR for the whole branch is similar, but reviews are a nightmare since it’s only a single review for the entire branch, which can be thousands of lines of code.

        At my workplace, we use feature flags (essentially if statements that can be toggled on or off) rather than feature branches. Everyone works directly from the main branch. We use continuous deployment which means landed code is quickly pushed to production. New features are hidden behind a feature flag until they’re ready to roll out.

        • iknowitwheniseeit@lemmynsfw.com
          1·
          9 months ago

          You can make a PR against your feature branch and have that reviewed. Then the final PR against your man branch is indeed huge, but all the changes have already been reviewed, so it’s just LGTM and merge that bad boy!

          • dan@upvote.au
            1·
            9 months ago

            You can make a PR against your feature branch and have that reviewed

            But what if you have multiple PRs that depend on each other? Or are you saying only have one PR open at a time? That sounds like it’d be very slow?

            • iknowitwheniseeit@lemmynsfw.com
              1·
              9 months ago

              I suppose it is possible to have two PR that have changes that depend on each other. In general this just requires refactoring… typically making a third PR removing the circular dependency.

              It sounds like your policy is to keep PR around a long time, maybe? Generally we try to have ours merged within a few days, before bitrot sets in.

              • dan@upvote.au
                1·
                9 months ago

                Sorry, my comment was unclear. I didn’t mean a circular dependency, just PRs that have a chain of dependencies (e.g. PR 100 that depends on 99, that depends on 98, that depends on 97)

                They’re usually not around for a long time, but there can be relatively large chains if someone is quickly adding new features.

  • Olgratin_Magmatoe@lemmy.worldEnglish
    8·
    9 months ago

    My first PR at my current job was about 130 files for the front-end, and about 70 for the backend. This hits close to home.