Skip to content

Conversation

@relivio
Copy link

@relivio relivio commented May 21, 2025

No description provided.

@rducasse rducasse self-assigned this May 21, 2025
Copy link

@rducasse rducasse left a comment

Choose a reason for hiding this comment

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

please update the changelog

@relivio
Copy link
Author

relivio commented May 24, 2025

Hi,

I did some changes in order to update the PR.
Tell me if it's ok ?

Also I see that run "verify" fails on my PR, I guess it's about rule specification. Do you know how I could correct this error ? Maybe I need to update rule specification or change the version of rule specification in the pom of my PR ?

Error: Tests run: 5, Failures: 0, Errors: 5, Skipped: 0, Time elapsed: 0.264 s <<< FAILURE! -- in org.greencodeinitiative.creedengo.java.JavaRulesDefinitionTest
Error: org.greencodeinitiative.creedengo.java.JavaRulesDefinitionTest.testAllRuleParametersHaveDescription -- Time elapsed: 0.173 s <<< ERROR!
java.lang.IllegalStateException: Can't read resource: org/green-code-initiative/rules/java/GCI91.html

Thank

@utarwyn
Copy link
Member

utarwyn commented May 24, 2025

Also I see that run "verify" fails on my PR, I guess it's about rule specification. Do you know how I could correct this error ? Maybe I need to update rule specification or change the version of rule specification in the pom of my PR ?

Error: Tests run: 5, Failures: 0, Errors: 5, Skipped: 0, Time elapsed: 0.264 s <<< FAILURE! -- in org.greencodeinitiative.creedengo.java.JavaRulesDefinitionTest Error: org.greencodeinitiative.creedengo.java.JavaRulesDefinitionTest.testAllRuleParametersHaveDescription -- Time elapsed: 0.173 s <<< ERROR! java.lang.IllegalStateException: Can't read resource: org/green-code-initiative/rules/java/GCI91.html

Thank

Hello @relivio!

Yes you will need to use the version of creedengo-rules-specifications that integrates your rule in order to build the project. In your local environment, you can modify the project by your own and integrate the updated jar. In the CI, you will need to wait for the artifact with specifications to be released.

@relivio
Copy link
Author

relivio commented May 25, 2025

Hello @relivio!

Yes you will need to use the version of creedengo-rules-specifications that integrates your rule in order to build the project. In your local environment, you can modify the project by your own and integrate the updated jar. In the CI, you will need to wait for the artifact with specifications to be released.

Hello,

Ok, so I'm waiting for the next release of rules-specification, on my local it's OK :)
And do you think I need to comment and resolve the requested change from the review ?

Olivier.

MethodInvocationTree mit = (MethodInvocationTree) tree;

if (!isTerminalOperation(mit)) {
return; // On ignore les appels intermédiaires (filter, sorted, etc.)
Copy link
Member

Choose a reason for hiding this comment

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

please put here comments in english ...

@dedece35
Copy link
Member

dedece35 commented May 25, 2025

@utarwyn @relivio
I've just released a new creedengo-rules-specification in 2.2.3 version.
It will be soon on maven central.
please check it with following URI and use it when it is present : https://central.sonatype.com/artifact/org.green-code-initiative/creedengo-rules-specifications/versions

@utarwyn
Copy link
Member

utarwyn commented Jun 6, 2025

@dedece35
I've just reeased a new creedengo-rules-specification in 2.2.3 version

I do not see it even after 2 weeks, do you know why?

@dedece35
Copy link
Member

dedece35 commented Jul 4, 2025

@utarwyn ... no, but I've just check the problem ...
this last previous action to deploy it to maven central is green ! but the last message warns me :

[INFO] Deployment 123015b2-2857-435c-9597-d2951368875a has been validated. To finish publishing visit https://central.sonatype.com/publishing/deployments

as asked for, I go to maven central web site and ... indeed, we have to click on "publish" button to do it effectively.
It's now ok ... the 2.2.3 version is now published and available !!!
sorry for delay

PS : maybe I deleted an option for automatic publish ... I will check it.

@dedece35 dedece35 self-assigned this Jul 4, 2025
@relivio
Copy link
Author

relivio commented Jul 4, 2025

@dedece Hello, sorry I have some full weeks of charge at work and at home :), I will push the correction requested this evening.

@relivio relivio requested review from dedece35 and rducasse July 4, 2025 20:55
int[] startLines = new int[]{25};
int[] endLines = new int[]{25};

checkIssuesForFile(filePath, ruleId, ruleMsg, startLines, endLines, SEVERITY, TYPE, EFFORT_5MIN);
Copy link
Member

Choose a reason for hiding this comment

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

when I execute the integration test, I have the following error and it's relevant for me. You should modify your startLines, endLines and severity value.

[ERROR] Failures:
[ERROR] GCIRulesIT.testGCI91:559->GCIRulesBase.checkIssuesForFile:84 [Extracted: rule, message, textRange.startLine, textRange.endLine, severity, type, effort]
Expecting ArrayList:
[("creedengo-java:GCI91", "Use 'filter' before 'sorted' for better efficiency.", 11, 14, MAJOR, CODE_SMELL, "5min"),
("creedengo-java:GCI91", "Use 'filter' before 'sorted' for better efficiency.", 16, 20, MAJOR, CODE_SMELL, "5min")]
to contain:
[("creedengo-java:GCI91", "Use 'filter' before 'sorted' for better efficiency.", 25, 25, MINOR, CODE_SMELL, "5min")]
but could not find the following element(s):
[("creedengo-java:GCI91", "Use 'filter' before 'sorted' for better efficiency.", 25, 25, MINOR, CODE_SMELL, "5min")]

Copy link
Member

Choose a reason for hiding this comment

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

the line 25 is the good one and not the bad one.

@dedece35 dedece35 requested a review from Copilot July 4, 2025 21:34

This comment was marked as outdated.

pom.xml Outdated

<!-- Version of creedengo rules specifications implemented by this plugin -->
<creedengo-rules-specifications.version>2.2.2</creedengo-rules-specifications.version>
<creedengo-rules-specifications.version>main-SNAPSHOT</creedengo-rules-specifications.version>
Copy link
Member

Choose a reason for hiding this comment

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

You can now use 2.2.3 version because it has been published a few minutes ago.

@dedece35
Copy link
Member

dedece35 commented Jul 4, 2025

Pull Request Overview

This PR introduces the new GCI91 rule to detect and warn when a sorted() call precedes a filter() in Java streams, as filtering before sorting is more efficient.

  • Implements the UseFilterBeforeSort rule logic and integrates it into the plugin
  • Adds unit tests, integration tests, and registers the rule in profiles and the registrar
  • Updates build config (plugin spec version, Java source/target) and documents the change in the changelog

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/test/java/org/greencodeinitiative/creedengo/java/checks/UseFilterBeforeSortTest.java Added unit test for the new rule
src/test/files/UseFilterBeforeSort.java Test fixture with compliant and noncompliant examples
src/main/java/org/greencodeinitiative/creedengo/java/checks/UseFilterBeforeSort.java Rule implementation and helper methods
src/main/java/org/greencodeinitiative/creedengo/java/JavaCheckRegistrar.java Registered the new rule
src/main/resources/org/greencodeinitiative/creedengo/java/creedengo_way_profile.json Enabled GCI91 in the default Sonar profile
src/it/test-projects/creedengo-java-plugin-test-project/src/main/java/.../UseFilterBeforeSort.java Sample code in the integration test project
src/it/java/org/greencodeinitiative/creedengo/java/integration/tests/GCIRulesIT.java Added integration test for GCI91
pom.xml Bumped rule-spec version to main-SNAPSHOT and set Java 16
CHANGELOG.md Documented the addition of GCI91
Comments suppressed due to low confidence (3)
src/main/java/org/greencodeinitiative/creedengo/java/checks/UseFilterBeforeSort.java:55

  • [nitpick] The inline comment is in French; consider translating it to English to maintain consistency with the codebase language.
        Collections.reverse(methodNames); // pour avoir l’ordre stream → ... → collect

src/it/java/org/greencodeinitiative/creedengo/java/integration/tests/GCIRulesIT.java:556

  • This integration test only asserts one violation at line 25 but there is a second noncompliant scenario starting at line 30. Please include line 30 in startLines and endLines arrays to fully validate the rule.
        int[] startLines = new int[]{25};

src/test/files/UseFilterBeforeSort.java:20

  • This test file references List and Collectors but lacks the necessary imports (java.util.List and java.util.stream.Collectors), causing a compilation error. Please add the missing import statements.
class UseFilterBeforeSort {

@relivio please take into account Copilot review feedbacks.

@relivio relivio requested a review from dedece35 August 4, 2025 12:37
@relivio
Copy link
Author

relivio commented Aug 4, 2025

Hello,

All checks have passed, but rule is considered as Major, don't hesitate to tell me if I need to change that.
Thank you.

Olivier

@dedece35 dedece35 requested a review from Copilot August 5, 2025 20:53
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a new static analysis rule (GCI91) that detects inefficient stream operations where sorted() is called before filter(), recommending the more efficient order of filtering before sorting to reduce the number of elements that need to be sorted.

  • Adds a new rule UseFilterBeforeSort that analyzes Java stream method chains
  • Implements comprehensive test coverage including unit tests and integration tests
  • Updates project configuration to include the new rule in the default profile

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/main/java/org/greencodeinitiative/creedengo/java/checks/UseFilterBeforeSort.java Main rule implementation that detects sorted() before filter() patterns
src/test/java/org/greencodeinitiative/creedengo/java/checks/UseFilterBeforeSortTest.java Unit test for the new rule
src/test/files/UseFilterBeforeSort.java Test file with compliant and non-compliant code examples
src/main/java/org/greencodeinitiative/creedengo/java/JavaCheckRegistrar.java Registers the new rule with the plugin
src/main/resources/org/greencodeinitiative/creedengo/java/creedengo_way_profile.json Adds GCI91 to the default rule profile
src/it/test-projects/creedengo-java-plugin-test-project/src/main/java/org/greencodeinitiative/creedengo/java/checks/UseFilterBeforeSort.java Integration test file with test cases
src/it/java/org/greencodeinitiative/creedengo/java/integration/tests/GCIRulesIT.java Integration test validation
pom.xml Updates dependencies and Java version configuration
CHANGELOG.md Documents the new rule addition
Comments suppressed due to low confidence (2)

src/test/files/UseFilterBeforeSort.java:39

  • The comment format is inconsistent. Compliant cases should not include the same message as non-compliant cases. This should be '// Compliant' without the violation message.
        list.stream() // Compliant {{Use 'filter' before 'sorted' for better efficiency.}}

src/it/test-projects/creedengo-java-plugin-test-project/src/main/java/org/greencodeinitiative/creedengo/java/checks/UseFilterBeforeSort.java:22

  • The comment format is inconsistent. Compliant cases should not include the violation message. This should be '// Compliant' without the message.
        list.stream() // Compliant {{Use 'filter' before 'sorted' for better efficiency.}}

return false;
}
String methodName = ((MemberSelectExpressionTree) tree.methodSelect()).identifier().name();
// Ajouter ici tous les terminaux connus
Copy link

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

The comment is in French. For consistency with the codebase, it should be in English: '// Add all known terminal operations here'

Suggested change
// Ajouter ici tous les terminaux connus
// Add all known terminal operations here

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

please correct this feedback

<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.13.0</version>
<configuration>
Copy link
Member

Choose a reason for hiding this comment

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

why this overriding of jdk versions ? it is already at 17...
I've just checked your PR with the deletion of this two overinf lines and UT et IT are OK.


<!-- Version of creedengo rules specifications implemented by this plugin -->
<creedengo-rules-specifications.version>2.2.2</creedengo-rules-specifications.version>
<creedengo-rules-specifications.version>2.2.3</creedengo-rules-specifications.version>
Copy link
Member

Choose a reason for hiding this comment

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

you can use the last one : 2.5.0

return false;
}
String methodName = ((MemberSelectExpressionTree) tree.methodSelect()).identifier().name();
// Ajouter ici tous les terminaux connus
Copy link
Member

Choose a reason for hiding this comment

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

please correct this feedback

int sortedIndex = methodChain.indexOf("sorted");
int filterIndex = methodChain.indexOf("filter");

if (sortedIndex != -1 && filterIndex != -1 && sortedIndex < filterIndex) {
Copy link
Member

Choose a reason for hiding this comment

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

are you sure about your algorithms ... I think in streams we can have several select method ... thus you can control only the first one ? or more complex, control all the stream and we want only control th second part ... for example :
stream
. filtrer()
.sorted()
. findAny() /// first part
. sort()
. filter()
. collect() // second part

what is the behaviour for examples with the same complexity (several parts) ?
can you add some such cases in your test resources files to control the behaviour ?

@github-actions
Copy link

github-actions bot commented Sep 5, 2025

This PR has been automatically marked as stale because it has no activity for 30 days.
Please add a comment if you want to keep the issue open. Thank you for your contributions!

@github-actions github-actions bot added the stale label Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants