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
base: master
Are you sure you want to change the base?
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.
@@ -58,6 +60,7 @@ public final class PhTraced implements Phi { | |||
/** | |||
* Locator of encaged object. | |||
*/ | |||
@SuppressWarnings("PMD.AvoidFieldNameMatchingMethodName") | |||
private final Integer locator; |
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 same here, just rename the variable
@@ -50,6 +50,7 @@ public final class PhWrite extends PhDefault implements Atom { | |||
* @param attr Attribute name | |||
* @param ret Return value function | |||
*/ | |||
@SuppressWarnings("PMD.ConstructorOnlyInitializesOrCallOtherConstructors") |
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 maybe we can rewrite the code instead of suppressing the warning?
// @checkstyle IllegalCatchCheck (3 line) | ||
} catch (final RuntimeException ex) { | ||
throw ex; | ||
} catch (final Throwable ex) { | ||
} catch (final Exception ex) { |
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 this seems to be a pretty dangerous change. Throwable
is of a higher level than Exception
- we must catch 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 I made it because of PMD.AvoidCatchingThrowable
violation. It seemed reasonable to me. As far as I understand, it is pointless to catch Error
, since it indicates irrecoverable situation. According to https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/java/lang/Error.html "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch."
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 we catch RuntimeException
two lines above, so here we can catch either Exception
either Throwable
- it would be the same.
@@ -44,6 +44,7 @@ public final class VerboseBytesAsString implements Supplier<String> { | |||
* Ctor. | |||
* @param data Data. | |||
*/ | |||
@SuppressWarnings("PMD.ArrayIsStoredDirectly") |
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 this is a reasonable complaint of PMD, I suggest we fix this
@@ -34,6 +34,7 @@ | |||
* | |||
* @since 0.16 | |||
*/ | |||
@SuppressWarnings("PMD.JUnit5TestShouldBePackagePrivate") |
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 not just fixing this error?
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 Temporarily this class should be public because it stores
public static final String TO_ADD_MESSAGE = "TO ADD ASSERTION MESSAGE";
There is todo about it: https://github.com/c71n93/eo/blob/88b3b4bf5e7cf54efd3a116aac7a41cc35ce54ef/eo-runtime/src/test/java/org/eolang/AtCompositeTest.java#L42. This decision was made here #3113 (review)
@@ -95,6 +96,7 @@ private static class Rnd extends PhDefault { | |||
/** | |||
* Ctor. | |||
*/ | |||
@SuppressWarnings("PMD.ConstructorOnlyInitializesOrCallOtherConstructors") |
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 this is also easily fixable
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 As I understand it is common practice to use PhDefault::add
in constructor of classes that extends PhDefault
and created for tests. I don't think that we need to fix it. @maxonfjvipon what do you think?
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 it's not a brightest design so we should fix it anyway. But it's not a part of this task. It should be done in other PR
@c71n93 most of the suppressions that you introduced are related to easily fixable code mistakes. I suggest we try to fix them instead of suppressing. |
public T get() { | ||
try { | ||
return this.origin.call(); | ||
} catch (final InterruptedException ex) { | ||
Thread.currentThread().interrupt(); | ||
throw new ExInterrupted(); | ||
} catch (final ExAbstract ex) { |
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 there was a reason why I added this ExAbstract
catch block but I forgot to document it. There are a lot of places in the code where we throw some exceptions that extends ExAbstract
. But today ExAbstract
extends RuntimeException
, tomorrow it may be just Exception
. So the main purpose of this block is to explicitly show that this method catches ExAbstract
exception which is important for such objects like PhSafe
or AtSafe
. Because they convert it to EOerror.ExError
exception
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 |
Closes #3160
PR-Codex overview
This PR focuses on code cleanup and improvements across multiple classes, including the addition of
@SuppressWarnings
annotations and constructor parameter updates.Detailed summary
@SuppressWarnings
annotations to classes for code quality improvements.SafeFunc
andExAbstract
.PhPackage
class.UniverseDefaultTest
.