8000 Add revision settings to Update Site by t8y8 · Pull Request #187 · tableau/server-client-python · GitHub
[go: up one dir, main page]

Skip to content

Add revision settings to Update Site #187

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

Merged
merged 4 commits into from
May 10, 2017

Conversation

t8y8
Copy link
Collaborator
@t8y8 t8y8 commented May 10, 2017

Addresses #186

(And helps me with some internal stuff)

Updated tests and manually tested internally

@t8y8 t8y8 requested a review from graysonarts May 10, 2017 00:48
@t8y8 t8y8 self-assigned this May 10, 2017
@LGraber
Copy link
Contributor
LGraber commented May 10, 2017

How come you didn't added a getter / setter for revisionLimit? How come these values are not read from get requests? I haven't checked the code to see if we are returning them but it seems like we would be.

@t8y8
Copy link
Collaborator Author
t8y8 commented May 10, 2017

I didn't change the model, it was already there so I didn't even check, I just added it to the serializer

@LGraber
Copy link
Contributor
LGraber commented May 10, 2017

Huh. Not good that we have a partial implementation like that. Oh well. Thanks for fixing! For your CL, though, it looks like we are simply naming the member "revision_limit" and not using a getter / setter. This doesn't match any of our other properties. Can you fix that? We might also debate on requiring revision_history_enabled to be true to set it but for now, I don't think that matters. Besides that it looks great.

8000
Copy link
Contributor
@graysonarts graysonarts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 🎸 🇫🇷

Copy link
Contributor
@LGraber LGraber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rocket

@t8y8 t8y8 merged commit 6f9dc37 into tableau:development May 10, 2017
@t8y8 t8y8 deleted the 186-feature-revision-settings branch May 10, 2017 21:01
t8y8 added a commit to t8y8/server-client-python that referenced this pull request Jun 28, 2017
Revision History settings were present in the SiteItem model but not actually serialized and didn't have setters/getters. I've updated that and enhanced the is_int decorator to allow exemptions in the case of sentinels that fall outside the normal range.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0