feature/checkstyles #149
feature/checkstyles #149
Conversation
|
@hosmanagic-axcient I thought you might provide interesting feedback for this one considering your previous comments on pull requests. |
From the top of my head, I think you need a rule which forbids interface names starting with a capital I.:D Just kidding. I appreciate you tagging me, I'll try to review at earliest! |
|
@hosmanagic-axcient Because there's already a rule against consecutive capital letters in interface/classes names, they all already trigger. We might suppress that rule though. |
| <message key="ws.notPreceded" | ||
| value="WhitespaceAround: ''{0}'' is not preceded with whitespace."/> | ||
| </module> | ||
| <module name="OneStatementPerLine"/> |
There was a problem hiding this comment.
I'm wondering if some useful ternary operator could be forbidden by this, something in the style of return obj != null ? obj.toString() : "no description". Then again, it might not even come often.
There was a problem hiding this comment.
I don't believe ternary expressions count as more than one statement. Looking at the documentation for the rule it looks like this rule is mostly looking for semicolon separation. But thanks for the heads up.
| COMPACT_CTOR_DEF"/> | ||
| <property name="allowNoEmptyLineBetweenFields" value="true"/> | ||
| </module> | ||
| <module name="SeparatorWrap"> |
There was a problem hiding this comment.
Of course, it's a matter of preference, but IIUC, this means that following is not correct
graph.users(userId)
.mailFolders(mfId)
.delta();Personally, I find that style a bit better, because it makes it clear that's a method call continuing from the previous line, and we're not calling a method from the same class.
There was a problem hiding this comment.
@hosmanagic-axcient meaning the dots would have to be at the previous line?
There was a problem hiding this comment.
@baywet Yes, that's my understanding based on this here: https://checkstyle.org/config_whitespace.html#SeparatorWrap
|
@baywet from what I can could check (I tried checking them one by one and looking up the definition), the rules look good to me. I added 2 minor comments. I believe we'll know more and better once we run the rules. That will likely be a process of polishing them over time.: ) |
|
@hosmanagic-axcient thank for giving it a look. Are you aware of any command line tool that would go ahead and fix all the trivial issues (indent, curly braces...)? So far my research has only brought back IDE plugins in intelliJ and such. |
This comment has been minimized.
This comment has been minimized.
baywet
force-pushed
the
feature/checkstyles
branch
from
b97afbf to
f03056a
Compare
Sorry for the late reply! Unfortunately, I don't know of an alternative. I've used my IDE (IntelliJ) to format code (at all levels: from code blocks to complete packages). |
|
This pull request has conflicting changes, the author must resolve the conflicts before this pull request can be merged. |
fixes #148
Context: This repo is lacking of linting + static analysis to improve code quality and maintainability.
At this point I haven't enabled build fail on lint fail because we still have another 6500+ linting issues in main.
I just wanted to get feedback on this tool which only does styling checks of things like indent, naming conventions, import ordering etc...
It does not run a deep static analysis and try to "understand what the code does" like other tools might. But it's really fast to run.
I think we could use a combination of tools, as long as they don't conflict with one another.
Before I go and furiously fix defects, I wanted to check with the team: