4 ms·

Saw the commit, its nice you improved the code quality too, wherever you touched by adding diamond brackets and making fields final.

koolba 9y ago
Refactoring for code quality improvements is a good thing but it shouldn't be bundled with changes like this. It makes it harder to see what actually changed. In this case there are 20 lines modified but only one (or four if you count deletions) is part of the core change. Unless the "final" qualifiers are required as part of this improvement, say to better inform the GC of what can be changed, then I'd suggest separating them out into separate commits. That way the lines that improves the hashCode() method and switches to using CompoundCacheKey stand out as the real changes.
- segmondy 9y ago
  Thanks for this comment. I've had this discussion over and over with other developers. Separate commits for separate things no matter how small. Doesn't matter if the code change is 1 line. Conflating multiple issues (even reformatting) with a bugfix brings a lot of noise to the diff and increases the review process.
  - johnou 9y ago
    In this case I feel changing the LRUCache to support generics was an integral part of the changeset to prevent possible regressions with cache usage (as I was changing the cache key from String to another type). Without it, it would be possible to use the incorrect object type in one of the many code paths, by adding generics we benefit from compile time checks.
- treve 9y ago
  I find that in practice, these types of changes tend to happen a lot less if they are forced in separate pull requests.
  - richardwhiuk 9y ago
    Separate commits is sufficient.
johannes1234321 9y ago
Indeed - thanks for taking the time, we really appreciate that! (I'm from MySQL's connectors team, while not working on Java)
johnou 9y ago
I added generics to ensure type safety, it is not technically diamond brackets (they came in jdk7 but mysql connector targets jdk6).

koolba 9y ago

Refactoring for code quality improvements is a good thing but it shouldn't be bundled with changes like this. It makes it harder to see what actually changed. In this case there are 20 lines modified but only one (or four if you count deletions) is part of the core change. Unless the "final" qualifiers are required as part of this improvement, say to better inform the GC of what can be changed, then I'd suggest separating them out into separate commits. That way the lines that improves the hashCode() method and switches to using CompoundCacheKey stand out as the real changes.

segmondy 9y ago
Thanks for this comment. I've had this discussion over and over with other developers. Separate commits for separate things no matter how small. Doesn't matter if the code change is 1 line. Conflating multiple issues (even reformatting) with a bugfix brings a lot of noise to the diff and increases the review process.
- johnou 9y ago
  In this case I feel changing the LRUCache to support generics was an integral part of the changeset to prevent possible regressions with cache usage (as I was changing the cache key from String to another type). Without it, it would be possible to use the incorrect object type in one of the many code paths, by adding generics we benefit from compile time checks.
treve 9y ago
I find that in practice, these types of changes tend to happen a lot less if they are forced in separate pull requests.
- richardwhiuk 9y ago
  Separate commits is sufficient.

johannes1234321 9y ago

Indeed - thanks for taking the time, we really appreciate that! (I'm from MySQL's connectors team, while not working on Java)

johnou 9y ago

I added generics to ensure type safety, it is not technically diamond brackets (they came in jdk7 but mysql connector targets jdk6).