Skip to content

Remove CDSView.source and infer the source from CDSView's parent#11773

Merged
bryevdv merged 2 commits intobranch-3.0from
mattpap/remove_CDSView.source
Nov 4, 2021
Merged

Remove CDSView.source and infer the source from CDSView's parent#11773
bryevdv merged 2 commits intobranch-3.0from
mattpap/remove_CDSView.source

Conversation

@mattpap
Copy link
Copy Markdown
Contributor

@mattpap mattpap commented Oct 30, 2021

Having a GlyphRenderer and its CDSView use different data sources is currently technically possible, but doesn't make sense. There is quite a bit of code making sure everything is configured correctly. All of that is unnecessary. This PR removes CDSView.source and adds a view for CDSView which infers the source from its parent.

@mattpap mattpap added this to the 3.0 milestone Oct 30, 2021
@bryevdv
Copy link
Copy Markdown
Member

bryevdv commented Oct 30, 2021

I'll have to go back and re-visit for details but noting that there was a lengthy discussion of view/filter syntax here that should be considered #11560 cc @p-himik

@p-himik
Copy link
Copy Markdown
Contributor

p-himik commented Oct 30, 2021

I'd have to look at it with quite a bit more care and also go back to the work I've been doing in the context of that view/filter discussion to provide any useful comments, but FWIW right now having a view for a view sounds like a step backwards - right into the uncanny valley of OOP.

@mattpap mattpap force-pushed the mattpap/remove_CDSView.source branch from a2dd321 to 456ad6f Compare October 31, 2021 10:02
@mattpap mattpap added status: ready and removed status: WIP experimental Not attempting to merge labels Oct 31, 2021
@mattpap
Copy link
Copy Markdown
Contributor Author

mattpap commented Nov 1, 2021

FWIW right now having a view for a view sounds like a step backwards - right into the uncanny valley of OOP.

This has nothing to do with OOP, just the design of bokehjs (having models and views).

@p-himik
Copy link
Copy Markdown
Contributor

p-himik commented Nov 1, 2021

Ah, after a more careful pass over the code I see that the two words "view" have different meaning in there.

With that being said, I still can't quite say if I would've done it the same way. But seems like this change is at the very least not detrimental to the ideas in #11560 and actually is probably beneficial.

@bryevdv bryevdv merged commit 805e2f7 into branch-3.0 Nov 4, 2021
@bryevdv bryevdv deleted the mattpap/remove_CDSView.source branch November 4, 2021 05:43
@mattpap mattpap mentioned this pull request Nov 17, 2021
6 tasks
bryevdv pushed a commit that referenced this pull request Dec 13, 2021
…11773)

* Remove CDSView.source and use renderer's source

* Remove unnecessary type casts
@github-actions
Copy link
Copy Markdown

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants