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

scalastyle:on Locale.ROOT missing for some toLowerCase and throwerror => unable to build #84

Closed
wants to merge 4 commits into from

Conversation

obar1
Copy link

@obar1 obar1 commented Jul 5, 2019

ZhitongDB and others added 3 commits June 30, 2019 20:53
…able paths

In this PR, I added exception handling for `forPath` method in `DeltaTable.scala`.

After the change, Delta Table will make sure create instance just for "Delta Source", other sources will throw an exception.

This PR is tested by `DeltaTableSuite.scala`

Closes #79

Closes #5599 from tdas/qy70ntky.

Lead-authored-by: Zhitong Yan <zhitong.yan@databricks.com>
Co-authored-by: Tathagata Das <tdas@databricks.com>
Co-authored-by: ZhitongDB <50844714+ZhitongDB@users.noreply.github.com>
Signed-off-by: Tathagata Das <tathagata.das1565@gmail.com>
GitOrigin-RevId: eb3a014714aacc1b97d165089e610186b1bb9f83
details
- added local where needed
- added better ignore to both condition of it
small fix to make build to  work
@databricks-cla-assistant
Copy link

databricks-cla-assistant commented Jul 5, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ obar1
✅ tdas
❌ Mario_Amatucci


Mario_Amatucci seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

} else {
// scalastyle:off throwerror
Copy link
Author

Choose a reason for hiding this comment

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

as it is it does not work
likely because it s an IF

@@ -157,7 +158,7 @@ object SchemaUtils {
def checkColumnNameDuplication(schema: StructType, colType: String): Unit = {
val columnNames = explodeNestedFieldNames(schema)
// scalastyle:off caselocale
val names = columnNames.map(_.toLowerCase)
val names = columnNames.map(_.toLowerCase(Locale.ROOT))
Copy link
Author

Choose a reason for hiding this comment

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

added

@@ -715,7 +715,8 @@ private[delta] object PartitionUtils {
def checkColumnNameDuplication(
columnNames: Seq[String], colType: String, caseSensitiveAnalysis: Boolean): Unit = {
// scalastyle:off caselocale
val names = if (caseSensitiveAnalysis) columnNames else columnNames.map(_.toLowerCase)
val names = if (caseSensitiveAnalysis) columnNames else
Copy link
Author

Choose a reason for hiding this comment

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

100 chars scalastyle

@zsxwing
Copy link
Member

zsxwing commented Jul 8, 2019

@obar1 Thanks for your contribution. Could you provide the command that can reproduce the style check error? I tested locally and didn't find any error.

@obar1
Copy link
Author

obar1 commented Jul 9, 2019

I checked out as it is - after git clone
using intellij community - sorry I am not expert of scalastyle
hope it helps

@obar1
Copy link
Author

obar1 commented Jul 9, 2019

the error scalastyle are related to the lack of proper LOCAL and the throw exception - my version is probably safer than current anyway - do a branch diff

@zsxwing
Copy link
Member

zsxwing commented Jul 11, 2019

So the issue is // scalastyle:off caselocale doesn't work for your IDE. Right? Did yo see any error if you just run build/sbt test?

@obar1
Copy link
Author

obar1 commented Jul 12, 2019

hi @zsxwing
it looks you integrated my proposed changes but you did not accept this
master...obar1:83-bug
right?

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