[GitHub] [cassandra-accord] belliottsmith commented on pull request #18: CASSANDRA-17112: Publish StrictSerializabilityVerifier so Cassandra may use, and fixed bugs found with the integration

[GitBox <gi...@apache.org>]

belliottsmith commented on PR #18:
URL: https://github.com/apache/cassandra-accord/pull/18#issuecomment-1370291003

   > The style changes match C* style, so unless Accord has a documented style guide that is different than C*, I am not applying my "own personal style" but rather using the projects...
   
   They are your personal interpretation. You have imposed four changes:
   
   1) You have introduced new lines for empty loop braces. The existing formulation was explicitly permitted by the style guide.
   2) You have modified the spacing for new lines for `toString()` methods. Your spacing is not specified anywhere in the style guide to my knowledge, and in general the multi-line guidance while detailed is explicitly crafted to permit a degree of author flexibility for better layout.
   3) Removal of spaces in javadoc. This is not provided for anywhere, that I know of, and again in some cases go explicitly against the authors' intent.
   4) Removing spacing before the semi-colon in a for-loop guard. This one you have a case for, but is in my opinion not explicitly stated in the style guide, as we only inherit these from the sun style guide which simply says they are of the form `for (expression; expression; expression)`, and since spaces are valid in expressions in Java, this does not prohibit the use of a space before the semi-colon, only enforces it afterwards. This one might be debated to be in your favour, but again, I do not think it rises to the level of warranting a rewrite of somebody's recently produced code without first speaking to them.
   
   If you want to overrule the original author, you should read the style guide very carefully, or at least speak to them first.
   
   > Correct, they are style changes. We talked about this a long time back; when touching code that doesn't match our style its fine to upgrade it to the current style...
   
   From the style guide:
   
   "Note that the project has a variety of styles that have accumulated in different subsystems. Where possible a balance should be struck between these guidelines and the style of the code that is being modified as part of a patch. *Patches should also limit their scope to the minimum necessary for safely addressing the concerns of the patch.*"
   
   Since none of the style changes were functional, this is contrary to the style guide.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org