I assume we're good to go here - I'll start creating and implementing ticket and update the wiki page.
Thanks, Stan On Mar 13, 2025 at 13:33 -0400, Stanislav Lukyanov <stanlukya...@gmail.com>, wrote: > Mirza, > > Yes, your link is correct. It looks like there was a typo in mine. > > Ivan, > > Re enforcing code style on review. > Yes and no - you should, of course, speak up if you see code style issues. > But you should not expect to see them if the TC is green. If there is a green > TC but still code style issues, I'd expect that the reviewer > > 1. adds a reference to the code style doc section that's violated; > 2. adds a link to a ticket to add the respective check to the static checks. > > These checks aren't difficult to add. I have sections 3 and 4 from the Code > Style doc covered now on my local, and I think the formatting is covered by > existing checks. If something is missing, and you see it in your next review, > just create a ticket ;) > > Re Async in API vs internal methods. > Agree, this is how I also implemented it. I'll update the doc once I see > there is a (lazy) consensus here. > > Thanks, > Stan > On Mar 13, 2025 at 08:54 -0400, Ivan Bessonov <bessonov...@gmail.com>, wrote: > > > Hi everyone! > > > > I have a few points in response. > > > > > > > It is assumed that if the PR passes the standard test suite, it adheres > > > > > to the code style; if not, it is a test suite problem. > > > > Such an approach makes sense in theory, but until we have enough rules, > > enforcing them through review is a viable option. > > > > > > > Enforce the *Async suffix in methods returning CompletableFuture as > > > > > described in the code style. > > > > Codestyle must be fixed, if it really says it the way you did. This applies > > to public API only, we must state that explicitly. > > Some internal methods must not have *Async suffix even if they return > > CompletableFuture, it is a case by case decision. > > > > > > чт, 13 мар. 2025 г. в 09:41, Mirza Aliev <alievmi...@gmail.com>: > > > > > > > Hello Stan! > > > > > > Thank you for your idea! > > > > > > Link in the description leads to a non-existing page, is it intentional? > > > What about this already existing [1] page then? > > > > > > [1] > > > https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide > > > > > > чт, 13 мар. 2025 г. в 08:45, Stanislav Lukyanov <stanlukya...@gmail.com>: > > > > > > > > > > > Hello Igniters, > > > > > > > > > > I'd like to propose some improvements related to code style and its > > > > > enforcement. > > > > > > > > > > The goal is to have sensible and easily enforceable rules added to the > > > > > official code style guide and static checks. This will reduce the need > > > > > > > for > > > > > > > > debate and iterations during code reviews. To achieve this, it is also > > > > > proposed to define a process for code style enforcement and evolution. > > > > > > > > > > # Process Additions > > > > > > > > > > Add the following to the [Java Code Style Guide]( > > > > > > > > > > > > https://cwiki.apache.org/coInfluence/display/IGNITE/Java+Code+Style+Guide > > > > > > > > ). > > > > > > > > > > - Static enforcement > > > > > - All code style rules should be added to the project build as static > > > > > checks in one of the plugins, such as Checkstyle or PMD. > > > > > - The reverse is also true -- all static analysis checks should be > > > > > reflected in the code style. > > > > > - To simplify development, the same rules should also be reflected, > > > > > when > > > > > possible, in the IDE code style configurations stored in the > > > > > repository. > > > > > - It's acceptable to suppress inspections when it makes sense (e.g., > > > > > > > with > > > > > > > > @SuppressWarnings). Such suppressions are approved as part of the code > > > > > review. > > > > > - When adding a new static check, it is expected that there may be a > > > > > large number of existing violations. While they should ideally be > > > > > > > resolved, > > > > > > > > it is acceptable to suppress them. It's better to add a check with > > > > > many > > > > > suppressions than not to add a check at all. > > > > > > > > > > - PR submission and review > > > > > - PR reviewers are strongly encouraged to create tickets for static > > > > > analysis improvements when the PR has code style violations. > > > > > - It is assumed that if the PR passes the standard test suite, it > > > > > adheres to the code style; if not, it is a test suite problem. > > > > > > > > > > - Making changes to the code style > > > > > - Any changes to the code style must be discussed on the dev list and > > > > > approved with lazy consensus. > > > > > - Since all static checks must be a part of the code style, adding a > > > > > new > > > > > static check must be discussed on the dev list (unless it is adding a > > > > > missing check for an existing rule). > > > > > > > > > > # New Rules and Checks > > > > > > > > > > Following the proposed process, below are proposals for adding new > > > > > rules. > > > > > > > > > > - **New rule, existing check:** There must be a newline symbol at the > > > > > end > > > > > of the file. This is already enforced but not in the code style. > > > > > > > > > > - **New rule and check:** Prohibit nullable collections. > > > > > It's a recommended best practice in Java to use empty collections > > > > > > > instead > > > > > > > > of nulls. > > > > > Currently, some of the Ignite 3 code base uses empty collections while > > > > > some use nulls, which creates ambiguity. > > > > > An easy way to enforce that components do not pass null for > > > > > collections > > > > > is to prohibit @Nullable on collection types. > > > > > > > > > > It is also proposed to add new checks for existing rules. This is > > > > > > > included > > > > > > > > mostly for illustration purposes—the proposed process does not > > > > > require a > > > > > dev list discussion for enforcing an existing rule with a static > > > > > check. > > > > > > > > > > - **New check, existing rule:** var usage > > > > > Enforce the var keyword usage as described in the code style. > > > > > > > > > > - **New check, existing rule:** Async methods > > > > > Enforce the *Async suffix in methods returning CompletableFuture as > > > > > described in the code style. > > > > > > > > > > I will be creating tickets for the new checks above shortly. > > > > > > > > > > Thanks, > > > > > Stan > > > > > > > > > > > > > > > > > > > > > -- > > Sincerely yours, > > Ivan Bessonov > >
