-
Notifications
You must be signed in to change notification settings - Fork 3
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
Improve foldMapMergeRedshiftSchemas signature #193
Conversation
e453821
to
0827504
Compare
Pull Request Test Coverage Report for Build 5902784011
💛 - Coveralls |
@@ -88,24 +88,24 @@ object ShredModelEntry { | |||
if (s == NullCharacter) "\\\\N" | |||
else s.replace('\t', ' ').replace('\n', ' ') | |||
|
|||
val extraCols = List( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you wrap the column name in quotes (e.g. """"schema_version""""
). Then later in rdb-loader you remove the quotes again:
ShredModelEntry.extraCols.map(_._1.replaceAll(""""""", ""))
You might consider changing extraCols
so that the column name is unquoted. This means RDB loader can be cleaner because you don't need to unquote. To compensate for this change, you could add the quoting somewhere inside showProps
.
But there is more to consider!
By making this change, you have made extraCols
part of the pulic-facing API of schema-ddl. So at the very least it should have scaladoc, and an explicit type annotation.
And then also ask your question.... is the type List[(String, String, String, String)]
a sensible type for a public-facing value? Is there an alternative way of typing it, e.g. define a new case class?
Or alternatively, you could make extraCols
private, and then define extraColNames
as public:
val extraColNames: List[String] = extraCols.map(_._1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Ian, just pushed a new commit on both this PR and rdb loader PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider #194 , but apart from that I'd be happy to approve this.
33b094d
to
bde34b0
Compare
hey @istreeter , I merged #194 into this branch and squashed all commits, does this look good to you? I'd like to proceed to the release and cut M1 |
Details at #192