Refactor triggerErrorAndCatch function #24181
Conversation
|
Comparing: e7d0053...309ceba Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
|
Hey @Jahb, thanks for running this analysis and submitting a PR. I'm going to close this for a couple reasons, I hope the explanations make sense. First, this code is in our example fixtures, where we expect users may copy/paste from the examples. In that case, we want users to be able to copy code the way that they would write naturally, not artificially refactored because of the duplication we would have by including multiple examples. For example, if you copied just one of the components, you wouldn't typically move the function outside of the class, so you wouldn't want to copy it from our example that way. Second, even if this wasn't in the examples, as a team we tend to value WET over DRY, so you will see a ton of duplication in the React codebase. I appreciate that this is somewhat controversial, but we've found that this style works well for us. Finally, we don't tend to merge any sort of PRs that only refactor, because these tend to have a high bug-to-value ratio, introducing subtle bugs that are hard to find for a marginally low value. Hope this makes sense, thanks again! |
I see, most of the PRs I made today were refactors I understand what you mean when you look at it in terms of bug-value ratio. Thanks for letting me know. |
Summary
For a university project, we ran some code quality analysis tools on the React repo, namely SonarQuabe .
This PR is fixing some bad code smells it detected in the fixtures/error-handing/index.js.
Code Smells Resolved:
triggerErrorAndCatch()was function identical in 2 places in the file. It's been refactored to a function and called when needed. To improve readability and maintainability.How did you test this change?
Running the testing suite resulted in no new failed tests.