#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 |
c71n93
force-pushed
the
3160-enable-pmd-in-eo-runtime
branch
from
7644b64
to
ff1c2ed
Compare
|
@maxonfjvipon could you check this one, please? |
|
@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.
@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.
@c71n93 why can't you just make the name shorter?
There was a problem hiding this comment.
@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.
@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.
@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.
@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.
@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.
@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.
@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.
@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.
@c71n93 why not just fixing this error?
There was a problem hiding this comment.
@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.
@c71n93 this is also easily fixable
There was a problem hiding this comment.
@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.
@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.
@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.
@maxonfjvipon do we need this class? It never used now.
There was a problem hiding this comment.
@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.
@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.
@c71n93 what was wrong with locator name?
There was a problem hiding this comment.
@maxonfjvipon it repeats the method name (prohibited by PMD)
|
@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
@SuppressWarningsannotations and constructor parameter updates.Detailed summary
@SuppressWarningsannotations to classes for code quality improvements.SafeFuncandExAbstract.PhPackageclass.UniverseDefaultTest.