-
Notifications
You must be signed in to change notification settings - Fork 2
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
Up package version #123
Up package version #123
Conversation
@melvinfolkers Is there a good way to test this before merging? After merging we can first use it with a release candidate. |
@erfannariman I don't see your comment anymore, but you mean like this with the |
Yes I removed the comment because it would actually introduce breaking changes already, so in this case it's not valid with the Azure packages. |
Ready for review, limited some packages from above, and replaced the deprecated parameter |
@erfannariman or @melvinfolkers can one of you check again? |
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.
Approved from my side, but note that some code is written by me also. Maybe @erfannariman can take a look as well.
@TimvdHeijden did you run the tests from the test suite?
@@ -197,7 +197,7 @@ def upload_to_blob(self): | |||
blob=f"{self.table_name}/{self.table_name}.csv", | |||
) | |||
|
|||
data = self.df.to_csv(index=False, sep="^", quotechar='"', line_terminator="\n") | |||
data = self.df.to_csv(index=False, sep="^", quotechar='"', lineterminator="\n") |
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.
side note: Ive created e a new issue #125 for making this an export to .parquet.
Yes see my comment from March 17, |
I think we need to set |
Done. Tests work, but thats because I already had 1.5.3 locally. and they did not work with pandas 1.4.3. So valid point. |
No description provided.