github_pr_tree icon indicating copy to clipboard operation
github_pr_tree copied to clipboard

Extra features for large PR workflow

Open q-willboulter opened this issue 4 years ago • 4 comments

I just moved to Github from Gitlab and this extension is really great, thanks for building it!

I often have to review very large pull requests and there are a few features that help me quite a lot in my workflow, so I cloned the project and had a go at adding them. I was wondering if you're interested in having any of these:

  • Option to filter by the full file path instead of just file name. I use this to review things like api, src/main and src/test separately rather than go file by file alphabetically
  • Option to filter out file diffs as well as the tree view (so the diffs on the right hand side always match what's in the tree). This means I can filter to e.g. api and then scroll through all the api related changes
  • A button to click "viewed" on all currently visible files. I use this to register that I've reviewed all the api stuff after scrolling through it, rather than clicking on each individual file

I put these features as optional settings in the options page, with them disabled by default.

Happy to raise a PR but wanted to check if any of these sound useful first. If not that's totally fine, I can see why you'd want to avoid the behaviour of the extension spilling out into other areas of the screen such as the file diffs! Also, I'm going to be using this a lot for work so I'll be more than happy to help maintain it

Below is a gif to show the features together. The benefits aren't very obvious on small pull requests, but I struggled to find really large PRs in open source projects!

Animation

q-willboulter avatar Sep 18 '21 14:09 q-willboulter

Hi! Thanks for all the ideas and showing it in the gif. Welcome to GitHub as well.

I like all 3 ideas and would love to see a PR for each one separately.

  1. The filtering should just be the default. It makes sense to filter by the full path.
  2. I love this. Filtering out the files as well makes a lot of sense. I'm wondering how this works with back and forward buttons of the browser as the URL may point at a hidden file.
  3. I'm less sure of how this will work. This is a bit specific but let's see how it works out. Can be opt-in via the options.

Small PRs = will be easier to verify, test and merge

berzniz avatar Sep 18 '21 21:09 berzniz

Okay great - I will get you over some PRs as soon as possible.

I will flip 1. so it's enabled by default, do you want me to add options for all of them still so they can be disabled/enabled?

For 2. my current implementation is really basic, I just set style.display = 'none' on all of the diffElement references that don't match the filter

  1. Might be less generally useful but when there's a subset of files that all have small/minor changes I like to skim them and then submit all so they are out of the way

I have a bunch of other ideas I'd like to try to but they get progressively more niche, these seemed like the most generally applicable 😀 The main other thing I would use if it was available:

Collapsible "advanced filters" section above the main search bar, which could contain:

  • number of lines changed range picker. I like to quickly eliminate all of files with minor changes sometimes and then get down to the ones with the big changes and review these in my IDE)
  • preset filters that can be customised in options html (e.g. user can set up text filters they use in their workflow via the options and then just click them instead of typing - mine might be api, service, test, main)
  • reset all filters button

q-willboulter avatar Sep 18 '21 21:09 q-willboulter

Great. I will review the PRs when they are available.

Regarding advanced filters - I can see an icon that opens up an filters popup/popout modal to accept the filters criteria. So this can also be its own feature

berzniz avatar Sep 19 '21 12:09 berzniz

Raised a few PRs, I will leave the button for checking all the boxes for now and see how I get on without it

wboult avatar Sep 19 '21 14:09 wboult