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

Implemented #109 #110 #123

Merged
merged 1 commit into from Jan 9, 2017

Conversation

Projects
None yet
4 participants
@r-richmond
Copy link

r-richmond commented Dec 23, 2016

Thought I'd take a stab at implementing my requests #109 #110. I noticed I was going to have merge conflicts with development so I merged ahead of time and wanted to make sure I followed the standards in that branch so I merged ahead of time.

Also tested on some of my workbooks (10.1) without issues.

@r-richmond

This comment has been minimized.

Copy link

r-richmond commented Dec 23, 2016

Please email me the CLA at your earliest convenience

@t8y8

This comment has been minimized.

Copy link
Contributor

t8y8 commented Dec 23, 2016

Thank you for your PR AND for including docs updates, @r-richmond!

Please update PR to merge against tableau:development -- it's currently pointing to master.

Once we have the CLA all processed I will do a proper review!

(And happy holidays!)

@r-richmond r-richmond changed the base branch from master to development Dec 23, 2016

@r-richmond

This comment has been minimized.

Copy link

r-richmond commented Dec 24, 2016

Emailed github@tableau.com with a signed copy of the CLA & Changed the destination branch.

Cheers & Happy Holidays too!

@benlower

This comment has been minimized.

Copy link
Contributor

benlower commented Dec 27, 2016

@t8y8 CLA received from Ryan

@t8y8
Copy link
Contributor

t8y8 left a comment

Thank you!

I've reviewed the code, and it's pretty straight forward, just a copy/paste of the port code.

I really appreciate the docstrings and the documentation updates.

Can you add some unittests that cover setting/getting those attributes (look at port's tests for inspiration).

One other thing, it looks like you branched from master which has the extraneous doc changes in it from @jdomingu -- rebasing on development in your fork and then force-pushing should resolve that. If you can't it's not a huge deal; I can ignore the unrelated changes

@r-richmond r-richmond force-pushed the r-richmond:109-110 branch from b7a06d7 to eba861f Jan 9, 2017

@r-richmond r-richmond force-pushed the r-richmond:109-110 branch from eba861f to 14b939c Jan 9, 2017

@r-richmond

This comment has been minimized.

Copy link

r-richmond commented Jan 9, 2017

Ok definitely pushed me beyond my comfort zone with git but I think I've done everything you requested, yay for growth.

Also added some tests, extended a couple to cover the new features, and tweaked the connection xml to include the query band. @t8y8 Let me know if this works for you.

@t8y8

t8y8 approved these changes Jan 9, 2017

@t8y8

This comment has been minimized.

Copy link
Contributor

t8y8 commented Jan 9, 2017

🚀 this same project has forced some git-fu upon me as well!
The changes look good, and the tests look good.

I'd like @RussTheAerialist to weigh in as well -- just because I know we're planning some work that would change how we update the document and the in-memory objects. Every new attribute we add is giving ourselves a teeny bit more work for that project... but given that this is simple, tested, and addresses a very common use case, I would be in favor of merging this.

@RussTheAerialist

This comment has been minimized.

Copy link
Contributor

RussTheAerialist commented Jan 9, 2017

@t8y8 I'm fine with 🚀 on this change. @r-richmond Thank you so much for doing this!

I know it will help more than a few people in the short term, and I'm not quite ready for the changes I'm planning to the internals. It does add some "cost" for me, but it also gives me a set of changes that help me validate the work that I'm hoping to finish this month :)

@t8y8 t8y8 added the enhancement label Jan 9, 2017

@t8y8 t8y8 merged commit cc97fc4 into tableau:development Jan 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@r-richmond

This comment has been minimized.

Copy link

r-richmond commented Jan 9, 2017

Awesome, happy to contribute. Just out of curiosity how often does the dev branch get merged into master? aka when should I expect to see my changes in the version i download from pip?

@RussTheAerialist

This comment has been minimized.

Copy link
Contributor

RussTheAerialist commented Jan 9, 2017

Usually we do it once a month, but since We didn't have one for December, I've been talking about cutting a release on Friday and I think everyone is supportive of that plan especially because of this change :)

RussTheAerialist added a commit that referenced this pull request Jan 11, 2017

Release 0.6 (#126)
* Fix #117 by only attempting files with the right extension inside the archive (#118)

* Commenting and Docstring cleanup. A few very small code cleanups (#120)

Add docstrings and remove clutter. I also made some very tiny tweaks to some code for clarity.

* Small cleanups for various editors. Play nice with built in test-runners (#121)

* Add Py36, update travis to use pycodestyle (#124)

* Add `initial sql` and `query band` support (#123)

Addresses #109 and #110

* Prep for release of 0.6 (#125)

* Prep for release of 0.6

* wordsmithing the changelog
@benlower

This comment has been minimized.

Copy link
Contributor

benlower commented Jan 12, 2017

@r-richmond @RussTheAerialist did this item fully address #109 and #110? If so, can those be closed? if not, what still needs to be done?

@t8y8

This comment has been minimized.

Copy link
Contributor

t8y8 commented Jan 12, 2017

Yup, those can be closed!

MrTomWhite added a commit to MrTomWhite/document-api-python that referenced this pull request Jun 11, 2017

Release 0.6 (tableau#126)
* Fix tableau#117 by only attempting files with the right extension inside the archive (tableau#118)

* Commenting and Docstring cleanup. A few very small code cleanups (tableau#120)

Add docstrings and remove clutter. I also made some very tiny tweaks to some code for clarity.

* Small cleanups for various editors. Play nice with built in test-runners (tableau#121)

* Add Py36, update travis to use pycodestyle (tableau#124)

* Add `initial sql` and `query band` support (tableau#123)

Addresses tableau#109 and tableau#110

* Prep for release of 0.6 (tableau#125)

* Prep for release of 0.6

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