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

"Quick Fixes" to Assorted IDE Warnings #3190

Closed

Conversation

currantw
Copy link
Contributor

@currantw currantw commented Dec 5, 2024

Signed-off-by: currantw [email protected]

Description

Cleanup. An assortment of "Quick Fixes" suggested by IntelliJ IDEA for warnings that it generates.

Related Issues

Cleanup on files I happened to open as part of #3145.

Check List

  • N/A New functionality includes testing.
  • N/A New functionality has been documented.
  • N/A New functionality has javadoc added.
  • N/A New functionality has a user manual doc added.
  • N/A API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • N/A Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@YANG-DB
Copy link
Member

YANG-DB commented Dec 5, 2024

@currantw pls resolve conflicts

@@ -174,7 +174,7 @@ public LogicalPlan visitRelation(Relation node, AnalysisContext context) {

@Override
public LogicalPlan visitRelationSubquery(RelationSubquery node, AnalysisContext context) {
LogicalPlan subquery = analyze(node.getChild().get(0), context);
LogicalPlan subquery = analyze(node.getChild().getFirst(), context);
Copy link
Member

Choose a reason for hiding this comment

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

please create a method visitFirstChild(node) and replace all the node.getChild().getFirst() calls

@currantw currantw force-pushed the opensearch-sql-3145_cleanup branch from 9af0daa to 3634175 Compare December 5, 2024 19:53
@andy-k-improving
Copy link
Contributor

I see there are different type of changes being apply in this PR( Ex: newer syntax of switch statement, getter substitution....etc) , should we separate these into multiple PR, under the same Github issue for the code clean up initiative?
This will make review easily along also for better tracking.

@YANG-DB
Copy link
Member

YANG-DB commented Dec 5, 2024

I see there are different type of changes being apply in this PR( Ex: newer syntax of switch statement, getter substitution....etc) , should we separate these into multiple PR, under the same Github issue for the code clean up initiative? This will make review easily along also for better tracking.

I agree with @andy-k-improving lets split this into smaller chunks ...

@currantw
Copy link
Contributor Author

currantw commented Dec 5, 2024

I see there are different type of changes being apply in this PR( Ex: newer syntax of switch statement, getter substitution....etc) , should we separate these into multiple PR, under the same Github issue for the code clean up initiative? This will make review easily along also for better tracking.

I'm not sure what the established process is for this (or if there is one), but my fear is that this would create a lot of overhead, and probably discourage developers (myself included) from taking on any "ad hoc" cleanup tasks if we need to create a new branch and new PR for each "type" of cleanup change.

These are "Quick Fixes" suggested by IntelliJ to warnings that the IDE itself generates, so they are intended to be uncontroversial changes. Moreover, these are just files that I happened to open (and notice these warnings) while working on other issues (rather than a comprehensive effort to fix one or more particular types of warning). My general thought is that we should be encouraging developers to do minor cleanup "as they go", rather than waiting for "big bang" dedicated cleanup work -- which tends to get done infrequently, if at all...

Will post on Slack to see if anyone else on the team has thoughts!

@currantw
Copy link
Contributor Author

currantw commented Dec 5, 2024

I see there are different type of changes being apply in this PR( Ex: newer syntax of switch statement, getter substitution....etc) , should we separate these into multiple PR, under the same Github issue for the code clean up initiative? This will make review easily along also for better tracking.

I agree with @andy-k-improving lets split this into smaller chunks ...

Okay, sounds good. I will close and create new separate PRs for each issue.

@currantw currantw closed this Dec 5, 2024
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.

3 participants