Skip to content

[MINOR][CORE] Remove unused variables, unused imports, etc.#24857

Closed
imback82 wants to merge 5 commits intoapache:masterfrom
imback82:unused_variable
Closed

[MINOR][CORE] Remove unused variables, unused imports, etc.#24857
imback82 wants to merge 5 commits intoapache:masterfrom
imback82:unused_variable

Conversation

@imback82
Copy link
Contributor

@imback82 imback82 commented Jun 12, 2019

What changes were proposed in this pull request?

  • Remove unused variables.
  • Remove unused imports.
  • Change var to val in few places.

How was this patch tested?

Unit tests.

@dongjoon-hyun dongjoon-hyun changed the title [CORE] Remove an unused variable in SparkSubmt.scala [MINOR][CORE] Remove an unused variable in SparkSubmt.scala Jun 12, 2019
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Thank you for your first contribution and welcome, @imback82 . I have a few comments.

  • We use [MINOR] tag if we don't have SPARK JIRAs.
  • Could you find more instances in core module? We usually recommend to do this kind of tasks module by module.

@imback82
Copy link
Contributor Author

  • We use [MINOR] tag if we don't have SPARK JIRAs.

Thanks. I will follow this from the next PR.

  • Could you find more instances in core module? We usually recommend to do this kind of tasks module by module.

I compiled with -Xlint:unused for the core module. I fixed most of the warnings for unused local variable and unused imports (there were some false positives since scaladoc was referring to the imports). I didn't fix unused private variable, private method, and private default arguments; there are about 50 of them and I can fix them in a separate PR if needed.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jun 13, 2019

Ur, thank you for the update, but let's remove unused imports stuff. You can get reviews later in another PR. It's good to have but sometime it's on the edge due to the intrusiveness. Also, it's beyond the scope of PR title.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

@imback82 . Please see the comments. This PR removes live codes.

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Except for keeping the private setActive method, I think this is fine as a general cleanup of dead code and imports

@imback82 imback82 changed the title [MINOR][CORE] Remove an unused variable in SparkSubmt.scala [MINOR][CORE] Remove unused variables, unused imports, etc. Jun 13, 2019
@dongjoon-hyun
Copy link
Member

ok to test

@dongjoon-hyun
Copy link
Member

I'll leave this PR to you, @srowen ~ Thank you, @srowen and @imback82 .

@SparkQA
Copy link

SparkQA commented Jun 13, 2019

Test build #106479 has finished for PR 24857 at commit f55f684.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@imback82
Copy link
Contributor Author

@srowen any update on this?

@HyukjinKwon I just noticed that you referenced this PR from #24858. Do you have the same concern with this PR? If messing with imports is a concern for you, I could revert those changes. Please let me know.

Thanks in advance!

@HyukjinKwon
Copy link
Member

It's fine since it fixes multiple instances. I see some committers are actively reviewing this.

@srowen
Copy link
Member

srowen commented Jun 15, 2019

Merged to master

@srowen srowen closed this in a950570 Jun 15, 2019
@imback82 imback82 deleted the unused_variable branch June 15, 2019 17:40
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.

5 participants