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.

Sign up for GitHub

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

Jump to bottom

Add checkstyle precommit for hidden/shadowed variables #19752

Open
dakrone opened this issue Aug 2, 2016 · 11 comments
Open

Add checkstyle precommit for hidden/shadowed variables #19752

dakrone opened this issue Aug 2, 2016 · 11 comments
Labels
:Delivery/Build Build or test infrastructure >enhancement Team:Delivery Meta label for Delivery team

Comments

@dakrone
Copy link
Member

dakrone commented Aug 2, 2016

Checkstyle has the ability to detect when variables are shadowed by local variables: http://checkstyle.sourceforge.net/config_coding.html#HiddenField

We should enable this as it makes reading the code clearer, as well as reducing potential bugs with missing this. prefixes.
@dakrone dakrone mentioned this issue Aug 2, 2016
Closed
@jasontedor
Copy link
Member

I assume that we would configure it so that this does not apply to constructor parameters nor to setter parameters?

@dakrone
Copy link
Member Author

dakrone commented Aug 2, 2016

Yep, those are options we can definitely enable

@colings86 colings86 added the :Core/Infra/Core Core issues without another label label Apr 24, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@nik9000
Copy link
Member

nik9000 commented Oct 15, 2018

@dakrone, do you still think this is worth it?

@danielmitterdorfer danielmitterdorfer added :Delivery/Build Build or test infrastructure and removed :Core/Infra/Core Core issues without another label labels Jan 29, 2019
@rjernst rjernst added the Team:Core/Infra Meta label for core/infra team label May 4, 2020
@mark-vieira mark-vieira added Team:Delivery Meta label for Delivery team and removed Team:Core/Infra Meta label for core/infra team labels Nov 11, 2020
@pugnascotia
Copy link
Contributor

Do we still want to do this? I enabled the HiddenField rule and even after tweaking the settings, it finds a vast number of errors.

We could enable it as a warning, or an IDE-only warning?

@dakrone
Copy link
Member Author

dakrone commented Jul 16, 2021

@pugnascotia I mean, I still want it, but I'm pretty opinionated about shadowing variables. Given that this is almost five years old, other people may have different opinions :D

@nik9000
Copy link
Member

nik9000 commented Jul 16, 2021

I guess there's two things:

  • Does it find really bad stuff. yes, I know we're going to have differing opinions on what is bad. But. I think we'll mostly agree on super bad. Right?
  • Is finding the super bad stuff worth the effort to remove the sorta-bad stuff. Or the pain of having another big list of checkstyle suppressions.

@dakrone
Copy link
Member Author

dakrone commented Jul 16, 2021

I think we'll mostly agree on super bad. Right?

For me shadowing is all about code readability. It's right up there with having a 140 character limit, sure we may not break things (well, in shadowing's case, we might), but it's so much easier to read code that doesn't shadow variables.

@pugnascotia pugnascotia self-assigned this Aug 11, 2021
@pugnascotia pugnascotia mentioned this issue Sep 10, 2021
Merged
pugnascotia added a commit that referenced this issue Sep 13, 2021
@pugnascotia
Fix shadowed variables in various places - part 1 (#77555)
e55edf9 
Part of #19752.

Fix a number of locations where local variables or parameters are shadowing a field
that is defined in the same class.
pugnascotia added a commit that referenced this issue Sep 13, 2021
@pugnascotia
Fix shadowed variables in various places - part 1 (#77555)
934691e 
Part of #19752.

Fix a number of locations where local variables or parameters are shadowing a field
that is defined in the same class.
@pugnascotia pugnascotia mentioned this issue Nov 9, 2021
Merged
pugnascotia added a commit that referenced this issue Nov 10, 2021
@pugnascotia
Fix HiddenField checkstyle problems in client/ (#80549)
cb26146 
Part of #19752.

Fix a number of cases of shadows vars under `client/rest-high-level`. As
part of this, fork the Checkstyle `HiddeFieldCheck` class so that it
understand the pattern of settings with no "set" prefix.
pugnascotia added a commit that referenced this issue Nov 10, 2021
@pugnascotia
Fix HiddenField checkstyle problems in client/ (#80549)
c2da6b1 
Part of #19752.

Fix a number of cases of shadows vars under `client/rest-high-level`. As
part of this, fork the Checkstyle `HiddeFieldCheck` class so that it
understand the pattern of settings with no "set" prefix.
@pugnascotia pugnascotia mentioned this issue Nov 15, 2021
Merged
pugnascotia added a commit that referenced this issue Nov 16, 2021
@pugnascotia
Fix shadowed vars pt3 (#80744)
1d3b096 
Part of #19752. Fix more instances where local variable names were shadowing field names.
@pugnascotia pugnascotia mentioned this issue Nov 18, 2021
Merged
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this issue Nov 30, 2021
@pugnascotia
Fix shadowed vars pt7 (elastic#80996)
8bee2cd 
Part of elastic#19752. Fix more instances where local variable names were
shadowing field names.

Also modify our fork of HiddenFieldCheck to add the ignoreConstructorBody
and ignoredMethodNames parameters, so that the check can ignore
more matches.
This was referenced Nov 30, 2021
Merged
Merged
elasticsearchmachine pushed a commit that referenced this issue Dec 2, 2021
@pugnascotia
Checkstyle shadows vars pt8 (#81147)
c1efe47 
Part of #19752. Fix more instances where local variable names were
shadowing field names. Also:    * Remove unused bits and pieces from
`XPackClientPlugin`.    * Expand the possible method names that are
skipped when       checking for shadowed vars, and allow shadowed vars
in       builder classes.
pugnascotia added a commit that referenced this issue Dec 2, 2021
@pugnascotia
Checkstyle shadows vars pt8 (#81147)
9950705 
Part of #19752. Fix more instances where local variable names were
shadowing field names. Also:    * Remove unused bits and pieces from
`XPackClientPlugin`.    * Expand the possible method names that are
skipped when       checking for shadowed vars, and allow shadowed vars
in       builder classes.
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this issue Dec 2, 2021
@pugnascotia
Checkstyle shadows vars pt8 (elastic#81147)
8e66148 
Part of elastic#19752. Fix more instances where local variable names were
shadowing field names. Also expand the possible method names that are
skipped when checking for shadowed vars, and allow shadowed vars in
builder classes.
@pugnascotia pugnascotia mentioned this issue Dec 2, 2021
Merged
elasticsearchmachine pushed a commit that referenced this issue Dec 2, 2021
@pugnascotia @elasticmachine
Fix shadowed vars pt7 (#80996) (#81144)
c23f8f6 
Part of #19752. Fix more instances where local variable names were
shadowing field names.

Also modify our fork of HiddenFieldCheck to add the ignoreConstructorBody
and ignoredMethodNames parameters, so that the check can ignore
more matches.

Co-authored-by: Elastic Machine <[email protected]>
elasticsearchmachine pushed a commit that referenced this issue Dec 2, 2021
@pugnascotia
Checkstyle shadows vars pt8 (#81147) (#81250)
6fc8166 
Part of #19752. Fix more instances where local variable names were
shadowing field names. Also expand the possible method names that are
skipped when checking for shadowed vars, and allow shadowed vars in
builder classes.
@pugnascotia pugnascotia mentioned this issue Dec 3, 2021
Merged
pugnascotia added a commit that referenced this issue Dec 6, 2021
@pugnascotia
Checkstyle shadows vars pt9 (#81296)
34a78c7 
Part of #19752. Fix more instances where local variable names were
shadowing field names.
pugnascotia added a commit that referenced this issue Dec 6, 2021
@pugnascotia
Checkstyle shadows vars pt9 (#81296)
e4e47c4 
Part of #19752. Fix more instances where local variable names were
shadowing field names.
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this issue Dec 6, 2021
@pugnascotia
Checkstyle shadows vars pt9 (elastic#81296)
3a5f641 
Part of elastic#19752. Fix more instances where local variable names were
shadowing field names.
This was referenced Dec 6, 2021
Merged
Merged
elasticsearchmachine pushed a commit that referenced this issue Dec 6, 2021
@pugnascotia
Checkstyle shadows vars pt9 (#81296) (#81359)
81ebcbc 
Part of #19752. Fix more instances where local variable names were
shadowing field names.
pugnascotia added a commit that referenced this issue Dec 7, 2021
@pugnascotia
Checkstyle shadows vars pt10 (#81361)
3e1fb1f 
Part of #19752. Fix more instances where local variable names were
shadowing field names.
pugnascotia added a commit that referenced this issue Dec 7, 2021
@pugnascotia
Checkstyle shadows vars pt10 (#81361)
5b138d5 
Part of #19752. Fix more instances where local variable names were
shadowing field names.
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this issue Dec 7, 2021
@pugnascotia
Checkstyle shadows vars pt10 (elastic#81361)
6df6380 
Backport of elastic#81361.

Part of elastic#19752. Fix more instances where local variable names were
shadowing field names.
@pugnascotia pugnascotia mentioned this issue Dec 7, 2021
Merged
elasticsearchmachine pushed a commit that referenced this issue Dec 7, 2021
@pugnascotia
Checkstyle shadows vars pt10 (#81361) (#81437)
6a25058 
Backport of #81361.

Part of #19752. Fix more instances where local variable names were
shadowing field names.
@pugnascotia pugnascotia mentioned this issue Dec 8, 2021
Merged
pugnascotia added a commit that referenced this issue Dec 21, 2021
@pugnascotia
Checkstyle shadowed vars pt11 (#81556)
2ad3f65 
Part of #19752. Fix more instances where local variable names were
shadowing field names.
pugnascotia added a commit that referenced this issue Dec 21, 2021
@pugnascotia
Checkstyle shadowed vars pt11 (#81556)
882728e 
Part of #19752. Fix more instances where local variable names were
shadowing field names.
@mark-vieira
Copy link
Contributor

@pugnascotia correct me if I'm wrong but I think we're all done here?

@pugnascotia
Copy link
Contributor

Nope, I just ran out of steam with fixing up instances of shadowed vars. I can pick it up again though.

@pugnascotia pugnascotia removed their assignment Jul 25, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
:Delivery/Build Build or test infrastructure >enhancement Team:Delivery Meta label for Delivery team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests