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

Fix csharpsquid:S1117 SonarCloud warnings about shadowing fields with variable names #1042

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

paulirwin
Copy link
Contributor

@paulirwin paulirwin commented Nov 21, 2024

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Fix csharpsquid:S1117 SonarCloud warnings about shadowing fields with variable names

Fixes #678

Description

This renames variables that shadowed fields of the same name, causing the SonarCloud S1117 warning. In the case of methods that replace static constructor initialization, this was named result instead of the field name. In other cases where there was no Lucene equivalent, the variable was renamed.

This also fixes a bug in FieldInfos where there was the possibility of a KeyNotFoundException due to a coding oversight.

This investigation discovered a potential maintainability bug in SortedSetDocValuesWriter.Flush. Due to GetOrdCountEnumerable and GetOrdsEnumerable (previously named with a typo) having the same return type, their names were accidentally swapped. I fixed the typo in the names and fixed the names to match upstream. I also confirmed that the signature of AddSortedSetField expected them in this order.

@paulirwin paulirwin added the notes:bug-fix Contains a fix for a bug label Nov 21, 2024
@paulirwin
Copy link
Contributor Author

After reviewing the changes again, I don't think the SortedSet code was actually buggy, the methods were just misnamed which caused confusion. Having the correct names will help avoid confusion in the future.

Copy link
Contributor

@NightOwl888 NightOwl888 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

It looks pretty good, but there were 4 lines I think we should change before merging. Please see the inline comments.

src/Lucene.Net/Support/CRC32.cs Outdated Show resolved Hide resolved
src/Lucene.Net/Index/SortedSetDocValuesWriter.cs Outdated Show resolved Hide resolved
src/Lucene.Net/Index/SortedSetDocValuesWriter.cs Outdated Show resolved Hide resolved
@paulirwin paulirwin merged commit ccff6dd into apache:master Nov 21, 2024
199 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes:bug-fix Contains a fix for a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename 'xxx' which hides the field with the same name
2 participants