Skip to content
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

no benefit in using guardlog. #23

Closed
wants to merge 1 commit into from

Conversation

soloturn
Copy link
Contributor

@soloturn soloturn commented Nov 1, 2023

jdrueckert commented on pr: MovingBlocks/Terasology#5154 that the rule is not really beneficial and better remove the rule than suppress it.

as well remove the not any more necessary comments.

jdrueckert commented on pr: MovingBlocks/Terasology#5154
that the rule is not really beneficial and better remove the rule than suppress
it.
soloturn added a commit to soloturn/terasology that referenced this pull request Nov 1, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23
soloturn added a commit to soloturn/terasology that referenced this pull request Nov 8, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23
soloturn added a commit to soloturn/terasology that referenced this pull request Nov 8, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23
soloturn added a commit to soloturn/terasology that referenced this pull request Nov 8, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23
soloturn added a commit to soloturn/terasology that referenced this pull request Nov 9, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>

squash later

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
soloturn added a commit to soloturn/terasology that referenced this pull request Nov 9, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
soloturn added a commit to soloturn/terasology that referenced this pull request Nov 9, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
soloturn added a commit to soloturn/terasology that referenced this pull request Nov 10, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
soloturn added a commit to soloturn/terasology that referenced this pull request Nov 10, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
@jdrueckert
Copy link
Member

Will be discussed in our next reviver meeting (tomorrow, Sunday 7PM UTC)

soloturn added a commit to soloturn/terasology that referenced this pull request Nov 11, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
@jdrueckert
Copy link
Member

we decided yesterday to keep the rule.
instead of the if guard, we can easily just go for substituting parameters like you already started doing in MovingBlocks/Terasology#5170, then the rule should not trigger.

@jdrueckert jdrueckert closed this Nov 13, 2023
@jdrueckert
Copy link
Member

There's an open issue on PMD regarding the GuardLogStatement rule - it's reporting false positives when using parametrized logs: pmd/pmd#4703
That's why the findings still show up.

@jdrueckert
Copy link
Member

Let's temporarily exclude the rule but with a TODO to reinclude it once the PMD issue (link it) is fixed.

@jdrueckert jdrueckert reopened this Nov 14, 2023
soloturn added a commit to soloturn/terasology that referenced this pull request Nov 17, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
soloturn added a commit to soloturn/terasology that referenced this pull request Nov 17, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
soloturn added a commit to soloturn/terasology that referenced this pull request Nov 17, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
@@ -7,7 +7,6 @@
Terasology PMD ruleset
</description>
<exclude-pattern>.*/org.terasology.protobuf/.*</exclude-pattern>
<!-- inline <rule ref="rulesets/java/basic.xml"/> -->
Copy link
Member

Choose a reason for hiding this comment

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

why are we removing these comments?

@jdrueckert
Copy link
Member

If the usage of suppliers sufficiently fixes the findings such that PMD doesn't report them anymore despite the open PMD bug, maybe we don't need this after all.

soloturn added a commit to soloturn/terasology that referenced this pull request Nov 17, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
soloturn added a commit to soloturn/terasology that referenced this pull request Nov 18, 2023
especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
@soloturn soloturn closed this Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants