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

[SEDONA-229] Remove Duplicate String Check #787

Merged

Conversation

douglasdennis
Copy link
Contributor

@douglasdennis douglasdennis commented Mar 6, 2023

Did you read the Contributor Guide?

Is this PR related to a JIRA ticket?

What changes were proposed in this PR?

This PR disables the scalastyle check called MultipleStringLiteralsChecker. This check is responsible for detecting multiple occurrences of the same string literal in the same file. From what I could gather, the reason for this check is to detect potential cases of "magic numbers" or constants that should be consolidated into a constant variable. There are three issues with this check:

  • There is a bug in the check which flags fragments of interpolated strings as duplicates even though the string as a whole is not a duplicate.
  • The main offender for this check are the tests. This is because of very common column names like "geom". The mass majority of these checks are false alerts. There was one instance of this check flagging non-test code and it provided little value to fix it.
  • Spark SQL uses a lot of strings and I wouldn't call it bad practice to do so.

This check did reveal some refactor points in the tests, but I would think that folks could already see that the tests could use some refactoring. In other words, this check doesn't flag anything that isn't obvious without it.

I did give a go at removing all duplicate strings for the sedona sql module and it led to some really ugly constant declarations in the test classes. Some nice refactors of the tests did fall out, and I will try to PR those once the linter messages are cleaned up.

How was this patch tested?

Ran the standard build process. This doesn't change any actual code.

Did this PR include necessary documentation updates?

  • No, this PR does not affect any public API so no need to change the docs.

@douglasdennis
Copy link
Contributor Author

Well, I was finally able to get to putting these PRs up. This is just one of several more to clean up the linter messages.

@jiayuasu jiayuasu merged commit 89916d8 into apache:master Mar 6, 2023
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.

None yet

2 participants