Merged
Conversation
* adding to ref pages; saving progress so far * Check in to save progress-o * Another chekin' for the cause * Check in more stuff; added api buckets in docs menu * Updated with users, groups; snapshot * Check in more changes to ref pages, minor fix for docs menu * more updates; formatting fixes, etc. * um updates, workbooks in prog, etc. * getting there, mostly done. * sorted, added filters, requests, etc. * ready for review.... * update to the left nav for new classes * Added sites.get_by_name; added quotes to sever.version number example * updates from tech review; added no_extract to downloads (workbook, data sources) * Added workbooks.update_connections (the cause of merge conflict); fixed some connectionitem info * tech review fixes, removed groups.get_by_id from examples, fixed datasource update example * Ran spellcheck, fixed misc errors
t8y8
approved these changes
Jun 27, 2017
| warnings.warn('no_extract is deprecated, use include_extract instead.', DeprecationWarning) | ||
| include_extract = not no_extract | ||
|
|
||
| if no_extract or not include_extract: |
Collaborator
There was a problem hiding this comment.
This can also be if not include_extract
| @parameter_added_in(no_extract='2.5') | ||
| def download(self, workbook_id, filepath=None, no_extract=False): | ||
| @parameter_added_in(include_extract='2.5') | ||
| def download(self, workbook_id, filepath=None, no_extract=None, include_extract=True): |
Collaborator
There was a problem hiding this comment.
For some reason I want to move include_extract to before no_extract so we just trim the end param, but this isn't mandatory, and others may disagree.
Collaborator
|
Ugh except there are conflicts with the base branch. Can you give me write access to your fork and I can try and take a look? |
Contributor
Author
|
I added you, @t8y8. Also made the formatting changes suggested and you can pull those in as well. |
Collaborator
|
Good news, I confirmed that it works manually and with tests. Bad news, the git-fu is beyond me, I think it's because d45's checkin originated from master, then was merged into development, and you're branched properly from development. @RussTheAerialist is the better git-fuer. But the code looks good to me. We should: 1) review tests and make sure no_extract isn't used anywhere and 2) update the docs, but given the merge conflicts, I say that can be a 2nd checkin |
…r-client-python into include_extract
t8y8
pushed a commit
to t8y8/server-client-python
that referenced
this pull request
Jun 28, 2017
Renames `no_extract` to `include_extract` for a clearer intent on what the parameter does. `no_extract` is kept but the default changes to `None` and we detect if it's used and throw a DeprecationWarning.
t8y8
pushed a commit
to t8y8/server-client-python
that referenced
this pull request
Jun 28, 2017
Renames `no_extract` to `include_extract` for a clearer intent on what the parameter does. `no_extract` is kept but the default changes to `None` and we detect if it's used and throw a DeprecationWarning.
bryceglarsen
pushed a commit
to bryceglarsen/server-client-python
that referenced
this pull request
Dec 28, 2023
## 08 (October 2021) * See dashboards in a workbook * Add shapes property * Add custom sql * Drop python 2, add up through 3.9 Co-authored-by: Tyler Doyle <t8y8@users.noreply.github.com> Co-authored-by: r-richmond <rrichmond.gh@gmail.com> Co-authored-by: Russell Hay <RussTheAerialist@users.noreply.github.com> Co-authored-by: Jared Dominguez <jdomingu@users.noreply.github.com> Co-authored-by: Kernpunkt Analytics <F.Eggert@kernpunkt.de> Co-authored-by: doulam <doulam@netflix.com> Co-authored-by: Kevin Glinski <kevin.glinski@Inin.com> Co-authored-by: Russell Hay <russell.hay@gmail.com> Co-authored-by: Dave Hagen <d45@users.noreply.github.com> Co-authored-by: martin dertz <martydertz@gmail.com> Co-authored-by: dev-mkc19 <mr.mkc19@hotmail.com> Co-authored-by: Jared Dominguez <jred51@gmail.com> Co-authored-by: Ben Lower <github@benlower.us> Co-authored-by: Patrick Carlson <carlson2442@gmail.com> Co-authored-by: Graeme Britz <gbritz@tableau.com> Co-authored-by: Jac Fitzgerald <jac.fitzgerald@salesforce.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This is a PR to change the parameter to include the extract from no_extract to include_extract. The logic changes from the default
no_extract=Falsetoinclude_extract=True. There is logic to convert the no_extract into the include_extract parameter and warn the user if it is used.