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

Issue #14448: Migrated IDEA to highest true scopes release v2022.3.3 #14696

Merged
merged 1 commit into from Apr 17, 2024

Conversation

MANISH-K-07
Copy link
Contributor

@MANISH-K-07 MANISH-K-07 commented Mar 20, 2024

Aims to close #14448

Follow-up of #14604 and checkstyle/contribution#837

Based on observations from #14604 (comment) and further discussions.

The latest IDEA release that we have used for update (2023.3.4) had issues with scopes.
A bit of research on different versions landed me on v2022.3.3 being stable and good with scopes.

Link to docker image (personal) for testing update :
https://hub.docker.com/layers/manishkk07/manish-k-07-checkstyle/jdk11-idea2022.3.3/images/sha256-3a659714655f8e033e648d894ac20eb110a0aa557f0f19821366cb54e4089305?context=repo

To pull image : docker pull manishkk07/manish-k-07-checkstyle:jdk11-idea2022.3.3

@MANISH-K-07

This comment was marked as outdated.

@MANISH-K-07
Copy link
Contributor Author

@romani , @nrmancuso

We have UnusedProperty firing violations on properties files.
Any suggestions on how we might suppress these? (instead of disabling the validation which is worst case alternative)

@romani
Copy link
Member

romani commented Apr 8, 2024

from CI:

<problem>
<file>
file://$PROJECT_DIR$/src/test/resources/com/puppycrawl/tools/checkstyle/main/
InputMainPropertyChainingUndefinedProperty.properties
</file>
<line>1</line>
<module>checkstyle</module>
<entry_point TYPE="file" FQNAME="file://$PROJECT_DIR$/src/test/resources/
com/puppycrawl/tools/checkstyle/main/InputMainPropertyChainingUndefinedProperty.properties"/>
<problem_class id="UnusedProperty" severity="ERROR" attribute_key="ERRORS_ATTRIBUTES">
Unused property</problem_class>
<description>Unused property 'mycheckstyle.severity'</description>
<highlighted_element>mycheckstyle.severity</highlighted_element>
<language>Properties</language>
<offset>0</offset>
<length>21</length>
</problem>
<problem>

it is again scope problems. Inputs should not be validated, as they weird by design.

<problems is_local_tool="true">
<problem>
<file>
file://$PROJECT_DIR$/src/test/resources/simplelogger.properties
</file>
<line>2</line>
<module>checkstyle</module>
<entry_point TYPE="file" FQNAME="file://$PROJECT_DIR$/src/test/resources/simplelogger.properties"/>
<problem_class id="UnusedProperty" severity="ERROR" attribute_key="ERRORS_ATTRIBUTES">
Unused property</problem_class>
<description>
Unused property 'org.slf4j.simpleLogger.log.com.tngtech.archunit.core.PluginLoader'
</description>
<highlighted_element>
org.slf4j.simpleLogger.log.com.tngtech.archunit.core.PluginLoader
</highlighted_element>
<language>Properties</language>
<offset>0</offset>
<length>65</length>
</problem>
<problem>
<file>
file://$PROJECT_DIR$/src/test/resources/simplelogger.properties
</file>
<line>3</line>
<module>checkstyle</module>
<entry_point TYPE="file" FQNAME="file://$PROJECT_DIR$/src/test/resources/simplelogger.properties"/>
<problem_class id="UnusedProperty" severity="ERROR" attribute_key="ERRORS_ATTRIBUTES">
Unused property</problem_class>
<description>
Unused property 'org.slf4j.simpleLogger.log.org.reflections'
</description>
<highlighted_element>org.slf4j.simpleLogger.log.org.reflections</highlighted_element>
<language>Properties</language>
<offset>0</offset>
<length>42</length>
</problem>

this looks like false positives. As this properties are for logging library, not for Checkstyle.

@MANISH-K-07
Copy link
Contributor Author

MANISH-K-07 commented Apr 8, 2024

@romani , Issue reported to jetBrains tracker at IDEA-351113

Shall we hold this PR meanwhile? Please let me know what you suggest?

@romani
Copy link
Member

romani commented Apr 8, 2024

Yes, Let's wait for their feedback.

@romani
Copy link
Member

romani commented Apr 8, 2024

It is sad to be blocked and not doing any progress forward.
https://www.jetbrains.com/idea/download/other.html

Did you try 2022.2.5 or 2022.3 ? At least we can upgrade to version before they damage scopes.

@MANISH-K-07
Copy link
Contributor Author

It is sad to be blocked and not doing any progress forward. https://www.jetbrains.com/idea/download/other.html

Did you try 2022.2.5 or 2022.3 ? At least we can upgrade to version before they damage scopes.

Yes @romani , label blocked doesn't ring good for me either :)

I will try to find another release supporting scopes, also keeping in mind the Yaml validation which is the main reason for this update.

@romani
Copy link
Member

romani commented Apr 8, 2024

Yaml validation instability , probably just another point to report to jetbrains

@MANISH-K-07 MANISH-K-07 changed the title Issue #14448: Migrated IDEA to highest true scopes release v2023.1.2 Issue #14448: Migrated IDEA to highest true scopes release v2022.3.3 Apr 16, 2024
@MANISH-K-07 MANISH-K-07 marked this pull request as ready for review April 16, 2024 02:53
@MANISH-K-07
Copy link
Contributor Author

@romani , @nrmancuso , 2022.3.3 is a stable release.. highest of 2022 tag

The PR has been updated for description, commit msg and suppressions resulting with new image.
Please air the image to complete migration :)

@romani
Copy link
Member

romani commented Apr 16, 2024

I am ok to merge.
Let's not update container until contribution PR is merged

@nrmancuso
Copy link
Member

@MANISH-K-07 please update image tag and make CI happy

@MANISH-K-07
Copy link
Contributor Author

MANISH-K-07 commented Apr 17, 2024

@MANISH-K-07 please update image tag and make CI happy

@nrmancuso , image updated :)

Rebased on master....

@MANISH-K-07
Copy link
Contributor Author

@nrmancuso , @romani , CI is green except for check_issues

@nrmancuso
Copy link
Member

@nrmancuso , @romani , CI is green except for check_issues

Ok, what do we need to do to make this green?

@MANISH-K-07
Copy link
Contributor Author

MANISH-K-07 commented Apr 17, 2024

@nrmancuso , @romani , CI is green except for check_issues

Ok, what do we need to do to make this green?

@nrmancuso , this is irrelevant to our code changes, I believe.

However, if this doesn't give you confidence, I will explain further....
The failing CI is because of an outdated linked issue
Funny thing is that the link causing problem is mentioned in your post at #14448 (comment), so yes, not done yet 😄

We need to edit by #14448 (comment) and restart CI to see a green suite

@nrmancuso
Copy link
Member

We need to try to enable this inspection again

@MANISH-K-07
Copy link
Contributor Author

MANISH-K-07 commented Apr 17, 2024

We need to try to enable this inspection again

Yupp, figured that out but wanted to confirm with you all @nrmancuso
#14448 (comment)

Will re-enable in new PR as minor once this is merged?
Is this commit good to merge @nrmancuso ?

@romani
Copy link
Member

romani commented Apr 17, 2024

Better to ree able in separate PR, and rerun CI numerous times before merge.

This PR is already big.

@MANISH-K-07
Copy link
Contributor Author

Better to ree able in separate PR, and rerun CI numerous times before merge.

This PR is already big.

Makes sense.. will try after this is merged :)

Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

@MANISH-K-07 please create a new issue to track reenabling the yaml validation inspection in this repo (not contribution) and send a minor PR to update the linked issue to make CI happy while we test the fix

@nrmancuso nrmancuso merged commit d37b821 into checkstyle:master Apr 17, 2024
112 of 113 checks passed
@MANISH-K-07 MANISH-K-07 deleted the idea branch April 17, 2024 17:37
@MANISH-K-07
Copy link
Contributor Author

@MANISH-K-07 please create a new issue to track reenabling the yaml validation inspection in this repo (not contribution) and send a minor PR to update the linked issue to make CI happy while we test the fix

Sure @nrmancuso .. will do

@MANISH-K-07
Copy link
Contributor Author

@nrmancuso , @romani , Please see #14806

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update version of IDEA inspection engine Version:2022.3.3
3 participants