[Sample List] Refactor Sample Grid Table [part 4] #1636
Conversation
There was a problem hiding this comment.
Another tough review 😅 — I tried, I really did, but the diff is just not reviewable (at least by me), sorry. 😞
SampleGridTableContainer has 1331 lines. Before even considering to convert it to a function component, I would first:
- open a PR to remove dead code;
- open another PR to refactor the filter logic in
SampleListViewContainer; - open yet another PR to replace the
deleteSamplesFromQueueaction creator withsetEnabledSample; - take one of the render methods in
SampleGridTableContainerand extract it into a new function component, making sure to clean things off in the process (refactor the code, access global state in the new component withuseSelector, etc.) — then open a PR again; - repeat for other render methods until
SampleGridTableContainerhas a more manageable size.
Those are just ideas but hopefully they get the point across. Refactoring is easy, but doing it safely (i.e. in a methodical, reviewable way) is not! The time you spend now will be saved later trying to debug regressions in production. 😁
I hear you the first time, now again I am Totally agree — refactoring is risky, especially in big files like this. But I think we can still do this in a single PR safely if we have a clear idea of the functionalities needed." That said, for this particular case i'd suggest we focus on testing than reviewing diff, I think doing the full refactor in a single, self-contained PR is actually safer and more maintainable long-term: Debugging is easier when changes are grouped in one place. If any regressions do appear, it's easier to trace and revert a single, focused PR than to hunt through a chain of smaller dependent PRs. “SampleGridTableContainer has a lot of cross-dependent logic. just transform to functional component make the dependent function not behave as before. transform just one method without seeing how it's being used across the rest introduces risk in isolation. By converting it as a whole, I can guarantee behavioral parity and ensure all the logic holds together.”
Yeah I do added 2 more commits that could be in another PR |
Sorry, I know I can be annoying. I'm just trying to help move the project towards better software engineering practices. I'm just one contributor of this project; others are free to disagree with my objections and approve your PR.
A PR review is first and foremost about reviewing code, not functionality. When I review, I make the assumption that you, as the developer, have thoroughly tested the changes that you've made. Sure it's always good if the reviewer has time to run the app and double check, but that's a bonus — the diff itself should tell me if any of the logic may have changed in an unexpected way. Can you give me the guarantee that you've checked every single logical branch of this huge component manually? If it were well covered by tests, I'd be a bit less worried — but even then, tests rarely cover every logical branch either (and they may have bugs themselves).
We can debate whether it's best to have one PR with multiple commits vs multiple PRs each with a single commit. That's a matter of opinion for sure, since there's little difference between the two approaches. But in both cases, the fact remains that commits should be atomic: they should each contain a single, self-contained, reviewable change. In the current state, most of the changes are in a single commit, so if there's a regression, you won't know which part of your refactoring caused it. With atomic commits, you'd have a lot more granularity. Let's take the commit that creates the
Transforming a class component to a function component should not introduce any behaviour change. Sure, it's handled differently in React's internals, but from our point of view, it's just another way of writing the same UI logic. In fact, class components are at the bottom of my list of priorities when I refactor. Sure they are the end goal, but the intermediary steps are so much more important! They include reviewing the current logic and simplifying it, removing dead code, extracting components and utilities, using modern syntax like destructuring, renaming confusing variables and methods, etc. It's only once a class component is small and manageable enough that it can be converted to a function component in an atomic commit. |
Thanks a lot for the thoughtful reply — I really appreciate the depth, It's great to have you that actually invested in code quality and long-term maintainability, which is rare and valuable. I am not pushing, or trying to say you are wrong. And that's what happened , i was not satisfy with the steps transformation, In this case i instinctively just get it done, especially because I understood the whole context better by doing (1 file - 1 refactoring - 1 commit). i try to avoid touching related file the most, but some are sometimes necessary. I'm definitely not saying that a full rewrite in one go is always the right approach — it's not. But for a component like this, where the priority is to introduce new structure rather than preserve or extend the existing one, I think doing it as 1 file → 1 refactor → 1 commit is a reasonable choice. In practice, it means:
The end goal — a better, safer, more modern component — is the same. This just felt like the most efficient and maintainable way to get there in this particular case.
I built this component it's of course easier for me to checked the content manually , but what i can not guarantee you, is that my checked only is enough to say we are good to go, even I tested the changes in one ESRF beamline this is not a guarantee that the changes does not contain regression (the user will tell). and we have to take into account that the other facilities have different equipment (Sample changer) that we simulate but they will have to properly test in real time. I am not a great reviewer but if i can't review diff, i review changes (the new code/file along), then the most important step stay the test. this component [SampleGridTableContainer ] was already deeply entangled before i rebuilt it 2 years ago, but unfortunately i was not able to do a cleaner job that time. thanks to the momentum you introduce i think now i can provide a better refractor, not saying this refractor is the ultimate tho. If the PR genuinely can't be reviewed by anyone — not even the beamline deployers — then it's fair to say I may need to close it. No hard feelings at all; it's part of our shared responsibility to make sure no one carries the risk alone. the progress should not kill the process <->. |
|
I'm still on vacation this week, just checking in on a few emails. Ill have a closer look at all this next week. I just wanted to say that I'm really happy to see such a nice discussion climate and good attitude. That is in my opinion something we should cherish :). Well I don't expect anything less as I know both of you to be really great guys ;) I know that its sometimes complicated to make a series of very clean PR's when we are refactoring and I really appreciate the work done. I agree that we should really strive towards atomic commits and PRs that are well described and easy to follow and understand. It is a good practice that helps us in our daily work/process and also to share knowledge and expertise. Ill see you all next week and we will have a closer look on how to approach this particular PR :) |
5144b04 to
46bf228
Compare
|
Ive tried to go through the PR and we have also tested it on a beamline and everything does indeed seem to be working as intended. Ill exceptionally merge this PR as quite some work as gone into this and that it would take a fair of amount of additional work to split it in several cleaner parts. |
This PR refactors the SampleGridTable and should close this issue
Transform to functional component
reduce filter cognitive complexity (getSampleListFilteredByCellPuck)
Decouple the function to reduce complexity and merge some function to avoid redundant logic
Some function can be extracted into smaller components later
Minimal CSS modifications, just enforce a max-width to avoid loosing responsiveness .
The diff in the [SampleGridTableContainer.jsx] are a bit big but making the changes in one PR is my opinion easier to deploy and debug / revert in case of regressions.
the file is much more simpler now and it will be more simpler when we extract the utility function and component .