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

Add support for string nodes to StaticLayoutProvider #13618

Merged
merged 24 commits into from Jan 4, 2024

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Dec 23, 2023

This PR allows to use string nodes with StaticLayoutProvider. Previously only integer keys were allowed. In future this could potentially be generalized to any types, but is out of scope for this particular work. As a side effect of this change, I had to figure out a solution to issue #13378, i.e. resolve deserialization issues of dictionaries. This PR partially supersedes PR #13437. I will refocus that PR from attempting to fix the issue to its actual main point, which is migration from plain objects to Map type. This PR also removes some leftovers after removal of hammerjs, which I noticed when dealing with some type issues.

fixes #12651
fixes #13378

@mattpap mattpap added status: WIP grant: SDG 2023 Funded from NumFOCUS Small Development Grant 2023 labels Dec 23, 2023
@mattpap mattpap added this to the 3.4 milestone Dec 23, 2023
Copy link

codecov bot commented Dec 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a2a15c8) 92.54% compared to head (a87efa7) 92.55%.
Report is 1 commits behind head on branch-3.4.

Additional details and impacted files
@@              Coverage Diff               @@
##           branch-3.4   #13618      +/-   ##
==============================================
+ Coverage       92.54%   92.55%   +0.01%     
==============================================
  Files             323      323              
  Lines           20493    20509      +16     
==============================================
+ Hits            18965    18983      +18     
+ Misses           1528     1526       -2     

@mattpap
Copy link
Contributor Author

mattpap commented Dec 29, 2023

The original half-measure approach in this PR didn't fully work, so I had to do things properly. This means allowing Map<string, T> | {[key: string}: T} where previously only {[key: string}: T} was allowed. This means no more hacks or implicit type coercions of any kind. Internally all properties that accept this border type use a proxying mechanism, so that all relevant code can assume Map interface and use get() or set() instead of index signatures and dreadful JS in operator. This in effect means that missing keys are finally handled properly.

This approach is different from what I attempted in PR #13437. There I converted plain objects to Map and tried to use asymmetric setters and getters to tie that on the type-level. This didn't work well, due to mutability of objects and that setters and getters with different types don't integrate well with the rest of TypeScript.

@mattpap mattpap force-pushed the mattpap/12651_string_nodes branch 3 times, most recently from dfc7110 to 7570d71 Compare December 30, 2023 15:35
src/bokeh/models/graphs.py Outdated Show resolved Hide resolved
examples/server/app/population.py Outdated Show resolved Hide resolved
@mattpap mattpap merged commit f821f2c into branch-3.4 Jan 4, 2024
29 of 30 checks passed
@mattpap mattpap deleted the mattpap/12651_string_nodes branch January 4, 2024 23:12
@mattpap mattpap mentioned this pull request Jan 25, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grant: SDG 2023 Funded from NumFOCUS Small Development Grant 2023 status: accepted
Projects
None yet
2 participants