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

Make Entity.ktElement no nullable #7259

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

BraisGabin
Copy link
Member

@BraisGabin BraisGabin commented May 5, 2024

There is not a good reason to keep Entity.ktElement nullable. It just make the API messier and provide no value.

Copy link

codecov bot commented May 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.76%. Comparing base (f92f8ab) to head (2326ebd).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7259   +/-   ##
=========================================
  Coverage     84.76%   84.76%           
+ Complexity     3992     3991    -1     
=========================================
  Files           578      578           
  Lines         12026    12022    -4     
  Branches       2477     2469    -8     
=========================================
- Hits          10194    10191    -3     
  Misses          606      606           
+ Partials       1226     1225    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BraisGabin BraisGabin added the breaking change Marker for breaking changes which should be highlighted in the changelog label May 5, 2024
@3flex 3flex added this to the 2.0.0 milestone May 6, 2024
3flex
3flex previously requested changes May 8, 2024
Copy link
Member

@3flex 3flex left a comment

Choose a reason for hiding this comment

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

This might not be a good idea:

  1. Not all rule violations relate to a KtElement, for example trailing whitespace (whitespace is represented by a PsiElement)
  2. Rule violations like MaxLineLength can include multiple KtElements

I haven't looked into it, but do we actually need to include the KtElement in the Entity at all? A location, signature and location should be enough to report on issues.

@BraisGabin
Copy link
Member Author

... do we actually need to include the KtElement in the Entity at all? A location, signature and location should be enough to report on issues.

We do. To implement the suppression feature. If KtElement is null the suppression doesn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api breaking change Marker for breaking changes which should be highlighted in the changelog build core rules suppressors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants