GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Implemented #109 #110 by r-richmond · Pull Request #123 · tableau/document-api-python

Conversation

@r-richmond

@r-richmond r-richmond commented Dec 23, 2016

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@r-richmond

r-richmond commented Dec 23, 2016

Copy link
Copy Markdown
Contributor Author

Please email me the CLA at your earliest convenience
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@t8y8

t8y8 commented Dec 23, 2016

Copy link
Copy Markdown
Contributor

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!)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@r-richmond
r-richmond changed the base branch from master to development December 23, 2016 23:42
@r-richmond

r-richmond commented Dec 24, 2016

Copy link
Copy Markdown
Contributor Author

Emailed [email protected] with a signed copy of the CLA & Changed the destination branch.

Cheers & Happy Holidays too!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benlower

benlower commented Dec 27, 2016

Copy link
Copy Markdown
Contributor

@t8y8 CLA received from Ryan
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
t8y8
t8y8 suggested changes Jan 7, 2017
View reviewed changes

@t8y8 t8y8 left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@r-richmond
implemented #109 #110 initial sql / query band & updated docs
14b939c
@r-richmond

r-richmond commented Jan 9, 2017

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
t8y8
t8y8 approved these changes Jan 9, 2017
View reviewed changes
@t8y8

t8y8 commented Jan 9, 2017

Copy link
Copy Markdown
Contributor

🚀 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@graysonarts

graysonarts commented Jan 9, 2017

Copy link
Copy Markdown
Contributor

@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 :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@t8y8 t8y8 added the enhancement label Jan 9, 2017
@t8y8
t8y8 merged commit cc97fc4 into tableau:development Jan 9, 2017
@r-richmond

r-richmond commented Jan 9, 2017

Copy link
Copy Markdown
Contributor Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@graysonarts

graysonarts commented Jan 9, 2017

Copy link
Copy Markdown
Contributor

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 :)
r-richmond reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
graysonarts pushed a commit that referenced this pull request Jan 11, 2017
Russell Hay
Release 0.6 (#126)
65fbc80 
* 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

benlower commented Jan 12, 2017

Copy link
Copy Markdown
Contributor

@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@t8y8

t8y8 commented Jan 12, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Yup, those can be closed!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Jan 20, 2017
Request: Initial SQL Read/Write Support #110
Closed
Request: Query Band Read/Write Support #109
Closed
@r-richmond
r-richmond deleted the 109-110 branch July 16, 2019 03:42
@r-richmond r-richmond mentioned this pull request Feb 19, 2023
⬆️ Allow more recent version of packaging tableau/server-client-python#1196
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@t8y8 t8y8 t8y8 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@r-richmond @t8y8 @benlower @graysonarts
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.