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> 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 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) > > > > > > > > >
