-
Notifications
You must be signed in to change notification settings - Fork 50
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
Update connection parameters #239
Conversation
As per the new changes in core, update default user name and database name to admin and omnisci respectively Similar updates to documentation Update connection parameters unit tests
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.
Since you are already making the changes, it feels like you should change the leading mapd
in the connection to omnisci, assuming that also works
After discussion with @andrewseidl and looking at the changes, It seems like a good idea to make the remaining |
3394942
to
9e0d22f
Compare
@@ -337,35 +337,53 @@ def test_dashboard_duplication_remap(self, con): | |||
"title": new_dashboard_name | |||
} | |||
} | |||
dashboard_id = "" | |||
dashboards = [] |
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.
What is the purpose of this empty dashboards list? Does this get overwritten later if you fall into the exception?
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.
Yes, it just a function scope placeholder I initialized to be used in different places throughout.
for dashboard in dashboards: | ||
if dashboard.dashboard_name == old_dashboard_name: | ||
dashboard_id = dashboard.dashboard_id | ||
try: |
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.
I feel like the try/catch is overkill here. Ultimately this problem is one of cleaning up after the tests. Other tests just check for the presence of the thing being created and drop it (like a table existing already), then re-create it. Here, we're nesting the logic into try/catch and duplicating the code, when it would be much simpler to just make the test either clean up after itself or drop the dashboard before trying to create it again.
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.
This is more efficient in present circumstances than always looping over all dashboards each time. Here we only use getdasboards call when we have to for each case. Regarding duplication, I can move them to separate function.
@mattdh Please refer to here for latest changes. |
@wamsiv I just pushed a commit with a few final changes containing updated db name. Looks like the changes are working against the latest version. Tests being ran both pip and conda. |
Danke @mattdlh! The PR can be merged once Travis test passes. I have verified them locally. |
As per the new changes in core, update default user name and database name to
admin
andomnisci
respectivelySimilar updates to documentation
Update connection parameters unit tests
cc: @mattdlh
I have not changed the connection URL to start with
omnisci
:It still starts with
mapd
left it to be in sync with repository namepymapd
. Please let me know if that should be changed as well.