Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/8753#issuecomment-140199709
(I think that one was closed from no activity, which maybe says something.
It was more about recording consistent default values?)
The upshot is certainly avoiding typos and there's no denying that helps. A
lot of typos would be caught because stuff breaks if the key is wrong, but you
never know.
A minor objection is simply that it's an extra layer of indirection, which
is itself a small cost paid repeatedly. My slightly bigger objection is that
this "spell checking" is being implemented as compile-time dependencies and
that causes some small trouble one way or another. If all your constants live
in one big file, then it has to be in a "core" module yet has constants for
every module in the project. If you localize constants, in my experience, you
eventually hit a point where you add a dependency across to another module just
to import its constants, which has a cost too. Or you duplicate the key anyway.
And you can't use this to avoid typos in non-code artifacts like docs. Better
than nothing but is it a false sense of security?
The poor practical argument against is that it's worth doing consistently
if at all, and doing it consistently could be a huge change.
Anyway that's why I personally don't like constants for constants. I'm sure
reasonable people disagree. Maybe there is a subset of the code where it's an
obvious win? lots of error potential, only in-module dependencies, code-only
references, etc
---
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
