Skip to content

Conversation

@gaborgsomogyi
Copy link
Contributor

What changes were proposed in this pull request?

JDBC connection providers implementation formatted in a wrong way. In this PR I've fixed the formatting.

Why are the changes needed?

Wrong spacing in JDBC connection providers.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing unit tests.

@gaborgsomogyi
Copy link
Contributor Author

cc @dongjoon-hyun @maropu
I thought it would be overkill to create a jira for this but if you think just let me know.

@HyukjinKwon HyukjinKwon changed the title [MINOR][SQL]Fix spaces in JDBC connection providers [MINOR][SQL] Fix spaces in JDBC connection providers Jun 29, 2020
@HyukjinKwon
Copy link
Member

It's actually not encouraged to make a PR for some trivial nits that are virtually same before/after in general given that we can just fix such nits later when we touch the codes for a fix together, and it uses the limited resources in Jenkins which abort the jobs globally when the number of jobs is too high for some reasons.

At least this PR fixes all style nits under connection packages non-invasively so I guess it's fine but let's avoid next time.

@gaborgsomogyi
Copy link
Contributor Author

Previously I've received suggestions not to pollute original PR intention w/ side effects like this. I'm basically fine w/ either way (but my vote goes definitely to your suggestion not to waste jenkins resources).

@srowen
Copy link
Member

srowen commented Jun 29, 2020

I don't know if we even have a strong convention for this, but I slightly prefer the double-indent for 'extends', to separate it from the body. Doesn't matter. If it's not consistent we might just leave it as is across the code.

@gaborgsomogyi
Copy link
Contributor Author

@dongjoon-hyun showed this: https://github.com/databricks/scala-style-guide/blame/master/README.md#L242

@dongjoon-hyun
Copy link
Member

This has been a known convention and I also have been encouraging to follow the rule in new PRs. However, I don't think this post-mortem PR is worth because the existing code was made by multiple commits. I usually recommend to use the guideline when the author touches that part, @gaborgsomogyi .

@gaborgsomogyi
Copy link
Contributor Author

OK @dongjoon-hyun , agreed. Should we proceed w/ this PR or should we melt them together?

@SparkQA
Copy link

SparkQA commented Jun 29, 2020

Test build #124642 has started for PR 28945 at commit e5f3562.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jun 30, 2020

Usually, @HyukjinKwon 's comment is the general consensus.

It's actually not encouraged to make a PR for some trivial nits

For this specific one, I'll merge this PR. Let's move on~ I believe we are all on the same page for the general rules.

@SparkQA
Copy link

SparkQA commented Jun 30, 2020

Test build #124653 has finished for PR 28945 at commit e5f3562.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants