Hi Jacek,
> Sometimes, although it would be just a single implementation, interface can > make sense for testing purposes - for mocking in particular This would surely mean there are two implementations, one of which is in the test tree? I think this is therefore already covered. > For exception handling, perhaps we should explicitly mention in the guideline > that we should always handle Exception or Throwable (which is frequently > being catched in the code) by methods from Throwables, which would properly > deal with InterruptedException I do not think this properly handles InterruptedException – InterruptedException that are not to be handled directly should now really be handled by propagating UncheckedInterruptedException, which is very different to catching all Throwable. In many cases InterruptedException should be handled explicitly, however. I do not think catching Exception or Throwable is the correct solution in most cases either – we should ideally only do so at the top level at which we want broad unforeseen problems to be handled, or where we need to take specific actions to handle exception, in which case we should ideally always rethrow the Throwable unmolested. I can see some benefit from explicitly outlining these cases, as it is not trivial to handle exceptions cleanly and correctly. We could perhaps create an exception handling addendum, perhaps in a separate page, that goes into greater detail? > I found it useful to access singletons by getInstance() method rather than > directly This can be beneficial for public use cases, but for private use cases it is oftentimes unhelpful to pollute the code. Also note that the document explicitly proposes avoiding getX, so we would instead have e.g. a method called instance(). Happy to add a section for this. >- "...If a line wraps inside a method call, try to group natural parameters >together on a single line..." while I'm generally ok with that approach, >putting each argument in a new line, makes it easier for git / review / >automatic merge I personally prefer to optimise for readability, and 10+ lines of single short parameters badly pollutes a page of code IMO. If there is no consensus on this we can put it to an indicative vote. >- imports - why mix org.apache.cassandra with other imports (log4j, google, >etc.)? I know that order is used for a while, but I was always curious why we >do that? I would be happy to revisit these, as we have not been consistent about imports in the codebase. I do not know why they are as they are. > - define continuation indent - currently it is 0 characters An opening brace introduces any necessary indentation (from the start of the line, which is perfect for legibility). I am somewhat inlined to declare that braces must be used if the lambda cannot fit on the declaring line. From: Jacek Lewandowski <lewandowski.ja...@gmail.com> Date: Monday, 14 March 2022 at 14:27 To: dev@cassandra.apache.org <dev@cassandra.apache.org> Subject: Re: Updating our Code Contribution/Style Guide Hi, I was looking at the document and have some thoughts: - Sometimes, although it would be just a single implementation, interface can make sense for testing purposes - for mocking in particular - For exception handling, perhaps we should explicitly mention in the guideline that we should always handle Exception or Throwable (which is frequently being catched in the code) by methods from Throwables, which would properly deal with InterruptedException - I found it useful to access singletons by getInstance() method rather than directly (public final static field). When we use getInstance() method, we may go further and make getInstance return the instance from a provider, which would by default return the value of final field. However in tests, it could return the value of some mutable static field from a custom provider. This way we would be able to easily switch the singleton which is impossible without reloading a class at the moment - "...If a line wraps inside a method call, try to group natural parameters together on a single line..." while I'm generally ok with that approach, putting each argument in a new line, makes it easier for git / review / automatic merge - imports - why mix org.apache.cassandra with other imports (log4j, google, etc.)? I know that order is used for a while, but I was always curious why we do that? - format configurations for IDEs - it seems like IntelliJ can import Eclipse formatter configuration, so maybe one configuration could be enough - define continuation indent - currently it is 0 characters - for unit test assertions - prefer AssertJ assertions over standard junit or hamcrest - maybe forbid them? AssertJ has much better descriptions of failing assertions I hope that we can enforce the rules using checkstyle, otherwise this effort may have little effect. For the transition, perhaps checkstyle could run on CircleCI just for the modified files? Thanks, jacek - - -- --- ----- -------- ------------- Jacek Lewandowski On Mon, Mar 14, 2022 at 1:10 PM Josh McKenzie <jmcken...@apache.org<mailto:jmcken...@apache.org>> wrote: we should add Python code style guides to it Strongly agree. We're hurting ourselves by treating our python as a 2nd class citizen. if we should avoid making method parameters and local variables `final` - this is inconsistent over the code base, but I'd prefer not having them. If the method is large enough that we might mistakenly reuse parameters/variables, we should probably refactor the met Why not both (i.e. use final where possible and refactor when at length / doing too much)? The benefits of immutability are generally well recognized as are the benefits of keeping methods to reasonable lengths and complexity. On Mon, Mar 14, 2022, at 7:59 AM, Marcus Eriksson wrote: Looks good One thing that might be missing is direction on if we should avoid making method parameters and local variables `final` - this is inconsistent over the code base, but I'd prefer not having them. If the method is large enough that we might mistakenly reuse parameters/variables, we should probably refactor the method. /Marcus On Mon, Mar 14, 2022 at 09:41:35AM +0000, bened...@apache.org<mailto:bened...@apache.org> wrote: > Our style guide hasn’t been updated in about a decade, and I think it is > overdue some improvements that address some shortcomings as well as modern > facilities such as streams and lambdas. > > Most of this was put together for an effort Dinesh started a few years ago, > but has languished since, in part because the project has always seemed to > have other priorities. I figure there’s never a good time to raise a > contended topic, so here is my suggested update to contributor guidelines: > > https://docs.google.com/document/d/1sjw0crb0clQin2tMgZLt_ob4hYfLJYaU4lRX722htTo > > Many of these suggestions codify norms already widely employed, sometimes in > spite of the style guide, but some likely remain contentious. Some > potentially contentious things to draw your attention to: > > > * Deemphasis of getX() nomenclature, in favour of richer set of prefixes > and more succinct simple x() to retrieve where clear > * Avoid implementing methods, incl. equals(), hashCode() and toString(), > unless actually used > * Modified new-line rules for multi-line function calls > * External dependency rules (require DISCUSS thread before introducing) > > >
