674909 create nested project#208
674909 create nested project#208t8y8 merged 10 commits intotableau:developmenttableau/server-client-python:developmentfrom jimbodriven:674909-create-nested-projectjimbodriven/server-client-python:674909-create-nested-projectCopy head branch name to clipboard
Conversation
t8y8
left a comment
There was a problem hiding this comment.
🚀
I just have small suggested changes
samples/create_project.py
Outdated
| return project_item | ||
| except TSC.ServerResponseError: | ||
| print('We have already created this project: %s' % project_item.name) | ||
| sys.exit() |
There was a problem hiding this comment.
Make this a sys.exit(1) so it reports an error back in bash scripting cases
samples/create_project.py
Outdated
| parser.add_argument('--server', '-s', required=True, help='server address') | ||
| parser.add_argument('--username', '-u', required=True, help='username to sign into server') | ||
| parser.add_argument('--site', '-S', default=None) | ||
| parser.add_argument('-p', default=None) |
There was a problem hiding this comment.
I realize the other samples may not have this, but can you give it a help text that says "this is the password" or something?
test/test_project.py
Outdated
| single_project = TSC.ProjectItem('test') | ||
| single_project._id = '1d0304cd-3796-429f-b815-7258370b9b74' | ||
| single_project._permissions = 'Test to check if permissions copied over.' | ||
| single_project._some_field = 'Test to check if fields are copied over.' |
There was a problem hiding this comment.
Why this change?
_some_field isn't an attribute of the model, actually, neither is _permissions.
I think you can just delete this from the test entirely, it's not testing anything interesting
There was a problem hiding this comment.
We were confused by this as well, at first. The whole point of the test is to verify that attributes not defined in the class definition get copied over to the new object. We update the name to make it more obvious.
t8y8
left a comment
There was a problem hiding this comment.
🚀
FYI, Jim and I talked offline, and I told him to remove the copy fields test. I actually want to have all models update from the returned object unless they require the copy (like tags on workbooks).
Longer term I think we should refactor the way we synchronize tags and call them with Pager/OnDemand just like we do for sub-nouns, that way everything is in sync and models aren't responsible for tracking themselves over updates.
/cc @RussTheAerialist
graysonarts
left a comment
There was a problem hiding this comment.
looks solid. Sorry for the delay, I'm in a branch rotation right now, so I have to ignore most of my day job 🎸 🚀
This was originally submitted by @kometes. We rebased on the development branch and resubmitted the pull request to development.