Skip to content

Adding support for checkstyle rules to enforce naming conventions#66

Merged
sfc-gh-pvillard merged 2 commits into
mainfrom
checkstyle-naming
Jul 8, 2026
Merged

Adding support for checkstyle rules to enforce naming conventions#66
sfc-gh-pvillard merged 2 commits into
mainfrom
checkstyle-naming

Conversation

@sfc-gh-pvillard

Copy link
Copy Markdown
Collaborator

No description provided.

@sfc-gh-dchaffelson sfc-gh-dchaffelson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, Pierre — had a proper read through the checkstyle naming rules. The AbstractNamingRule factoring is clean: centralising pattern / per-flow-override / component-exclusion resolution keeps each concrete rule tight, and the no-op-when-unconfigured behaviour means they're safe to leave in include: by default.

A few small things before it goes in:

  1. Rebase + CI re-run. The branch is a few months behind main now and CI last ran against the old base. The rules/parameter/ package move and the DefaultCheckstyleRules enum look like the likely conflict points, so worth a fresh rebase + green run.
  2. Cover the nested-process-group path. flow_v7_naming.json has processGroups: [], so the recursive descent into child groups (ProcessorNamingRule / ControllerServiceNamingRule) isn't exercised. Since nested groups are the norm in real flows, a fixture with a processor + controller service inside a child group would lock that behaviour in.
  3. (Optional) Guard the patterns cast. In AbstractNamingRule.getPatterns() the patterns value is cast straight to Map. If it's mistyped as a scalar in YAML it throws a ClassCastException that surfaces as an action crash with no comment posted, rather than a clear config error — an instanceof check with a "'patterns' must be a map" message would make misconfig friendlier. Non-blocking.

Two doc nits if you're in there anyway: a line noting patterns use String.matches() full-match semantics (so .*_SUFFIX$ rather than _SUFFIX$), and that a per-flow override replaces the whole patterns map rather than merging.

Overall looks good — happy to see it merge once rebased.

@sfc-gh-pvillard

Copy link
Copy Markdown
Collaborator Author

Thanks @sfc-gh-dchaffelson - pushed a commit to address your feedback

@sfc-gh-dchaffelson sfc-gh-dchaffelson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — to be clear, the two items in my review comment (a rebase + CI re-run, and a nested-process-group test fixture) are worth doing but I'm treating them as non-blocking follow-ups rather than gating this. Nice work on the rule framework.

@sfc-gh-pvillard sfc-gh-pvillard merged commit 14d0df8 into main Jul 8, 2026
3 checks passed
@sfc-gh-pvillard sfc-gh-pvillard deleted the checkstyle-naming branch July 8, 2026 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants