Skip to content
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

Use kwarg for Theme filename #5190

Merged
merged 2 commits into from
Aug 9, 2021
Merged

Conversation

bryevdv
Copy link
Contributor

@bryevdv bryevdv commented Aug 9, 2021

There was a recent change on Bokeh branch-2.4 to make the json and filename args to Theme be kwarg-only, but this is incompatible with Dask current usage. I will revert the change in Bokeh to maintain current compatibility, but I am also submitting the change here to set the stage for eventually re-introducing kwarg-only at some point in the future.

Secondarily, I have questions about what to do about testing? This was not caught by the "downstream" tests that Bokeh runs. A simple no-op test that just performs the import would have caught this since the use of Theme is at module scope. It's probably a good idea to make sure all of the bokeh-related modules cleanly import. But all of the bokeh related tests still seem to be in the dask repo, not this one. Should those tests be moved here? Or should I open a separate PR against dask to add tests?

Edit: reverted on Bokeh side at bokeh/bokeh#11484

@mrocklin
Copy link
Member

mrocklin commented Aug 9, 2021

+1 from me. I'm also happy bumping up the minimum version number higher if you would prefer.

@bryevdv
Copy link
Contributor Author

bryevdv commented Aug 9, 2021

I'm also happy bumping up the minimum version number higher if you would prefer.

I"m glad to hear this and I think I will have some improvements to memory and reference cycles in the very near future that will make bumping the min version even more compelling. But even so, in the immediate term I think making the forward-change here while delaying the change on the Bokeh side for a little while will avoid any version-related user pain.

I would definitely like to expand the bokeh-related testing regardless, to help keep on top of things. So any guidance about where those tests should live is appreciated.

@mrocklin mrocklin merged commit b0eefbd into dask:main Aug 9, 2021
@mrocklin
Copy link
Member

mrocklin commented Aug 9, 2021

Thanks @bryevdv ! This is in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants