4 ms·

Ask HN: How to efficiently review a pull request that is full of linter changes?

• lantry 4y ago
  tldr: don't review linter changes Run the new linter on all the files. Don't make any other changes. Spot check a few places to make sure the linter doesn't majorly mess anything up. Make sure your automated tests still pass. Merge this change without reviewing every single line. Now that all your files are "clean", continue making changes and reviewing them like normal.
  • zikohh 4y ago
    This has been my approach in the past but after reviewing it, I've found it to be sometimes inconsistent.
    • zikohh 4y ago
      Consistently inconsistent. But it took me ages to figure this out. Hence me looking for an enhanced way to review
• gus_massa 4y ago
  It's a public repo or a PR inside the company where you work? Anyway, the solution is to ask nicely that the author rewrites the PR with the relevant changes, and then make another PR with the new linter. (Which linter to use, is a project-wide decision. Who is in charge of this?)
  • zikohh 4y ago
    The PR only contains linter changes. However, there are so many it's very difficult review. I've nearly finished but I hate doing things manually and therefore was trying to find a more efficient way to do this in future
    • zikohh 4y ago
      It's an internal repo. Not public
• zikohh 4y ago
  I'd like to add the most linters change things like line length and so on. However, this linter is changing the order of keys and values in a go struct (which I'd already formatted with go fmt), which makes it much harder to review than your typical line length change.
  • gus_massa 4y ago
    > this linter is changing the order of keys and values in a go struct Is this a good idea?!?! I don't know golang. Can you show an example where this change is useful? (Or a link to a webpage that explains this change.) [Sorry for the side question, but I'm curious.]
    • zikohh 4y ago
      https://pastebin.com/Cdif0t5v https://pastebin.com/Cdif0t5v It changes the initialisation order - it's not a problem but hard to review.
    • zikohh 4y ago
      Oh wait useful ?! I don't think ordering structs alphabetically is useful I agree that if it's not useful it shouldn't be done and transitively that makes the pull request easier to review. But still a very indirect approach
      • gus_massa 4y ago
        Can it be disabled with an option? If I have an structure with mass, height and width, I'll really hate to order them as height, mass and width. My totally uninformed opinion is to run away from a linter that makes that changes.
• KolenCh 4y ago
  If there's a recipe to produce the change, ask for the recipe instead (and better yet, put it in the commit.) Then if you can reproduce that PR, and tests are passing, skim through it and see if nothing funny. Then it should be good to go.