-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#3160 Enable PMD for eo-runime
#3171
#3160 Enable PMD for eo-runime
#3171
Conversation
…c/test/java/org/eolang
…c/main/java/org/eolang
…ome new PMD violations
@maxonfjvipon could you check this one, please? |
@maxonfjvipon oops, it looks like something went wrong with PMD check on windows. Looks like internal PMD error. Have you ever come across something like this?
|
@c71n93 no, I use macos) |
@maxonfjvipon me too. But unfortunately we have windows CI :( |
@c71n93 did you try to find the solution on stackoverflow? |
@maxonfjvipon not yet. But according to #3160 (comment) there was some problems with PMD and |
7644b64
to
ff1c2ed
Compare
@maxonfjvipon could you check this one, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c71n93 that's awesome! Thanks
@yegor256 could you check this one, please? |
@yegor256 could you merge this one, please? |
@@ -35,6 +35,7 @@ | |||
* @since 0.22 | |||
*/ | |||
@Versionized | |||
@SuppressWarnings({"PMD.TooManyMethods", "PMD.ConstructorShouldDoInitialization"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c71n93 I believe, you can easily fix this error (that PMD complains about), instead of suppressing it
public final class PhTraced implements Phi { | ||
|
||
/** | ||
* Name of property that responsible for keeping max depth. | ||
*/ | ||
@SuppressWarnings("PMD.LongVariable") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c71n93 why can't you just make the name shorter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yegor256 Because this variable means "maximum cage recursion depth". As for me the shortest name for this variable is MAX_CAGE_RECURSION
. However max length of variable is two words. Any two-word options don't seem informative enough to me (MAX_RECURSION
, MAX_CAGE
, CAGE_RECURSION
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c71n93 how about RECURSION_THRESHOLD
? The name of the class already provides a lot of semantic: PhTraced.RECURSION_THRESHOLD
-- sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxonfjvipon do we need this class? It never used now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c71n93 we don't need it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yegor256 @maxonfjvipon maybe we can move this class to eo-runtime/src/test/java/EOorg/EOeolang/
? (it uses only there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c71n93 I believe it should stay in org.eolang
since it's kind of part of common runtime object which are used for building the concrete ones
@maxonfjvipon @yegor256 I resolved all I could. |
@yegor256 @maxonfjvipon could you check this one again, please? |
@yegor256 @maxonfjvipon could you check this one, please? I think it would be nice to enable pmd for eo-runtime as early as possible. |
@@ -58,7 +60,7 @@ public final class PhTraced implements Phi { | |||
/** | |||
* Locator of encaged object. | |||
*/ | |||
private final Integer locator; | |||
private final Integer locatr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c71n93 what was wrong with locator
name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxonfjvipon it repeats the method name (prohibited by PMD)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
@yegor256 could you check this one, please? |
@c71n93 one small comment above and we are good to go |
@yegor256 done. could you check it please? |
@yegor256 could you check this one, please? |
2 similar comments
@yegor256 could you check this one, please? |
@yegor256 could you check this one, please? |
@yegor256 I updated this branch. Could we finally merge it? |
@rultor merge |
@c71n93 thanks! |
Closes #3160
PR-Codex overview
This PR focuses on code cleanup and improvements.
Detailed summary
PhWrite
andPhOnce
@SuppressWarnings
annotations for PMD violationsSafeFunc
class