Skip to content

Conversation

@stijndehaes
Copy link
Contributor

Previously all partition columns where of the string type. The same
logic for non partition columns is reused to find out the type of the
partition column.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Previously all partition columns where of the string type. The same
logic for non partition columns is reused to find out the type of the
partition column,
@stijndehaes
Copy link
Contributor Author

stijndehaes commented Sep 19, 2019

@igorborgest I have some issue understanding how to best add tests for this. Could you point me in the right direction?

@igorborgest igorborgest self-requested a review September 19, 2019 11:45
@igorborgest igorborgest self-assigned this Sep 19, 2019
@igorborgest igorborgest added the enhancement New feature or request label Sep 19, 2019
@igorborgest
Copy link
Contributor

Hey @stijndehaes!

Awesome idea! Let's do that!

Your implementation is pretty good, and works fine in my tests here.
The only problem is that I've just updated the same code lines for other feature and now we have to resolve the conflicts.

I've already organized that forking your fork and submitted a PR for your branch.
Please accept that. 😄

@igorborgest
Copy link
Contributor

@stijndehaes, regarding the tests:

This project requires some setup for tests, and we tried to explain that here.

But basically, we must have a AWS environment (Cloudformation) and a local environment (Docker). Almost all tests are about end-to-end integrations between AWS and Local.

So after the setup of both ends, you only need to add test cases on testing/test_awswrangler.

Updating branch, resolving conflicts, adding more tests
@stijndehaes
Copy link
Contributor Author

@igorborgest I will try to set it up and execute them tomorrow :)

@igorborgest igorborgest merged commit 0d2a0af into aws:master Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants