jvm-profiling-tools / honest-profiler

A sampling JVM profiler without the safepoint sample bias
https://github.com/RichardWarburton/honest-profiler/wiki
MIT License
1.25k stars 146 forks source link

New Feature : Show/hide columns in tables for easier reading #197

Closed PhRX closed 7 years ago

PhRX commented 7 years ago

Another friday night special, I'll have to run to catch my train.

Enjoy !

RichardWarburton commented 7 years ago

Hi @PhRX,

This PR still has a delta of over 4000 lines of code. And #196 and #195 are pretty big as well. They all change multiple things in a single PR. I thought we had agreed to try and pull request smaller, more atomic changes?

PhRX commented 7 years ago

Hi @RichardWarburton,

This PR actually only is about the last commit., and contains +- 1000 lines. The issue seems to stem from my current workflow, which is :

This ensures that any new changes I make after the PR do not end up in the PR. However, it means that the commits from previous PRs end up my new branches, so each consecutive PR looks larger, since it accumulates the commits from previous PRs (so #197 includes the commits from PRs #196, #194 and #192).

If the PRs are treated "in order", the actual diffs only affect one feature or issue and are fairly small (about 1000 lines each it seems).

I'm not sure there's a good alternative. The only obvious one would be for me to always "go back to start", i.e. create branches off of what is by then for me essentially an "old version". That way, PRs would be strictly parallel. But that approach has more issues than benefits, esp. from my point of view.

If you can recommend a better way, please do, I'm trying to make things as easy as possible for everyone.

RichardWarburton commented 7 years ago

Hi @PhRX,

Ahh right, thanks for explaining that, makes a lot more sense now. I will just go through reviewing the PRs one after another. If they directly build upon changes from the previous PR then there's probably no better way of doing things. If they are independent changes entirely then you should be able to branch from the master before the commits in the other PR were made, which I think would help on this front.

PhRX commented 7 years ago

Hi @RichardWarburton,

Originally I was planning to add a note to the PRs indicating that they were consecutive (after I saw they turned out to be "cumulative" but somewhere along the way I forgot. So, my mistake.

Branching from the "old" version makes sense for reviewers but it is quite impractical from the coder perspective ;

The PR model implementation in git seems to be oriented towards many developers making small changes (which does correspond to the usual pattern when a team builds a product). This case is the exact converse : a single developer making many changes (large and small).

On the + side I'm running out quickly of larger features I need to see implemented, so the calvary is nearing its end ;) Future updates are likely to be local and very small (10s of lines rather than anything measurable in klocs).

Xeers Philip

RichardWarburton commented 7 years ago

Yeah, I can understand that. Great to hear that things are beginning to get nearer to your ideal state of play.

PhRX commented 7 years ago

Superseded by upcoming, cleaner PR.