Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/13329#issuecomment-222256093
In roughly descending order, here are possible positive results of a change:
- Fix a bug
- Improve performance
- Improve tests
- Clarify user-visible documentation (log/exceptions, javadoc)
- Improve code style or consistency
- Improve code comments or other non-functional clarification
and here are some possible costs or risks:
- introduces a bug
- introduces a performance regression
- adds dependencies
- adds to maintenance overhead
- decreases code consistency
- introduces non-trivial merge conflict problems
- takes time to review and merge
It's not that there aren't positive changes here, in isolation, but they're
just marginal. Although interpolation is probably better than format(), it's
inconsistent both before and after this change. Same with nonEmpty. Some of
these I don't think are improvements, like "fetch" -> "fetches".
But why wouldn't you want to merge any helpful change? It's really the cost
of reviewing it, and distantly, the merge conflict issue. I don't mind people
getting started by contributing small changes, and paying that cost in order to
on-board people into a process and community. I also think the occasional tiny
change is fine.
I should also say that I do find it reasonable to fix up code in these
small details when already changing it for other purposes.
What I don't want to do is fix 1 issue piecemeal over 10 changes. Either we
shouldn't really bother, or should fix it one go as much as possible. For
example, although we should probably talk about it, I'd be more in favor of
changing all `!...isEmpty` to `nonEmpty` in one go than this change.
I also prefer to see experienced contributors "graduate" towards more
significant changes or reviewing/shepherding other changes.
I have heard from a few sources that some organizations may incentivize
employees to contribute to Spark (good) and measure by number of JIRAs or pull
requests (bad). I don't think that's what's happening here, but another reason
I personally want to push back on _everyone_ submitting bitty changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
