Skip to content

Conversation

@vogelsgesang
Copy link
Collaborator

As part of #889, I changed the default value for the site argument to be None
instead of an empty string. This accidentally broke auth, as TableauAuth
expected an empty string instead of None to signify an absent site argument.

This commit fixes the issue by now actually accepting None as site_id in
TableauAuth and PersonalAccessTokenAuth, thereby making our interface more
intuitive, at least in my opinion.

Fixes #924

…rsonalAccessTokenAuth`

As part of tableau#889, I changed the default value for the `site` argument to be `None`
instead of an empty string. This accidentally broke auth, as `TableauAuth`
expected an empty string instead of `None` to signify an absent `site` argument.

This commit fixes the issue by now actually accepting `None` as `site_id` in
`TableauAuth` and `PersonalAccessTokenAuth`, thereby making our interface more
intuitive, at least in my opinion.

Fixes tableau#924
@t8y8
Copy link
Collaborator

t8y8 commented Oct 22, 2021

Technically “” isn’t an “empty” site id, it’s the site id for the default site. I believe we didn’t choose default as the actual ID for back-compat and because we localize default in the UI.

It’s a sin that we’ve been paying for since 7.0 when sites were introduced :(

I don’t think it’s safe to default to None throughout the library, as there might be places that look for ”” when serializing the xml, and None would skip the element

@t8y8
Copy link
Collaborator

t8y8 commented Oct 22, 2021

Also the slack error is back and blocking test runs. We shouldn’t reintroduce the slack message until we know it will be stable.

@jacalata
Copy link
Contributor

jacalata commented Nov 9, 2021

I don't think this should break anything, the change doesn't affect further than the code shown since it will set the value to '' anyway.

jacalata
jacalata previously approved these changes Nov 9, 2021
@jacalata jacalata changed the base branch from master to development January 28, 2022 08:13
@jacalata
Copy link
Contributor

updated so it only has the same type failures as the development branch, will merge into there.

@jacalata jacalata merged commit ce781da into tableau:development Jan 28, 2022
jacalata pushed a commit that referenced this pull request Mar 30, 2022
…rsonalAccessTokenAuth` (#925)

* Accept `None` as parameter for the `site_id` of `TableauAuth` and `PersonalAccessTokenAuth`

As part of #889, I changed the default value for the `site` argument to be `None`
instead of an empty string. This accidentally broke auth, as `TableauAuth`
expected an empty string instead of `None` to signify an absent `site` argument.

This commit fixes the issue by now actually accepting `None` as `site_id` in
`TableauAuth` and `PersonalAccessTokenAuth`, thereby making our interface more
intuitive, at least in my opinion.

Fixes #924
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.

"TypeError: cannot serialize None" in login.py

3 participants