You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
IMO, a strong point of altair is that because API generation takes place in the language of
it's users - the bar is relatively low for new contributors.
For example, there are 12 authors for the most recent 30 commits to vega/altair/tools
Making this change would lower the barrier to who can maintain this code1 and hopefully
increase the chances of a bug reporter being able to fix their issue themselves.
Surely LLMs can bridge the gap for those unfamiliar with JS?
Aha, got you!
If a potential contributor doesn't know JS, are they in a position to judge the correctness of LLM
output in that language?
Why did I already do it?
Note
Everything from here onwards is explaining my rabbit hole(s)
Feel free to stop reading now!
A couple of weeks ago I was looking into fixing (#1095 (comment)) as part of (#1088).
I'm embarrassed to admit it, but it took me quite some time to wrap my head around generate-python-api.js.
The lack of typing made the process pretty painful, as my IDE wasn't able to fill in the gaps.
To get myself out of that pickle, I added some JSDoc 2 and then began porting the rest in python to figure out what I needed to change.
// Keywords that cannot appear as a bare keyword-argument name. `type` is a
// builtin, not a reserved word, so it stays valid as a kwarg.
constPYTHON_KWARG_UNSAFE=newSet(
[...PYTHON_KEYWORDS].filter(k=>k!=='type')
);
I only noticed this because I used keyword.iskeyword4, which correctly allows type as a
keyword, but that also made me realize there is no coverage for either 😬
2 - transform-keys.js
This one confused me, because it lists every transform as they all have "a generated vgplot Python
API function."
I'm very confused by this solution. Nothing about the code
suggests that this is what's happening. Does this make sense to anyone else?
My questions here are:
Why isn't there a vg.format?
If there was, then TRANSFORM_KEYS would be a noop
Can we drop this part of the API generation and handle whatever this is doing in a clearer
way?
Either by adding the missing format helper
Or identifying when {"format": {... should be generated by using something named FORMAT*
3 - Docstrings
I hope it isn't too controversial to say that this is a tough read 😉
Why?
Why should we do it?
IMO, a strong point of
altairis that because API generation takes place in the language ofit's users - the bar is relatively low for new contributors.
For example, there are 12 authors for the most recent 30 commits to
vega/altair/toolsMaking this change would lower the barrier to who can maintain this code 1 and hopefully
increase the chances of a bug reporter being able to fix their issue themselves.
Surely LLMs can bridge the gap for those unfamiliar with JS?
Aha, got you!
If a potential contributor doesn't know JS, are they in a position to judge the correctness of LLM
output in that language?
Why did I already do it?
Note
Everything from here onwards is explaining my rabbit hole(s)
Feel free to stop reading now!
A couple of weeks ago I was looking into fixing (#1095 (comment)) as part of (#1088).
I'm embarrassed to admit it, but it took me quite some time to wrap my head around
generate-python-api.js.The lack of typing made the process pretty painful, as my IDE wasn't able to fill in the gaps.
To get myself out of that pickle, I added some JSDoc 2 and then began porting the rest in python to figure out what I needed to change.
Note
This first rabbit hole led to a second one - which sidetracked me for over a week -
before continuing on this one a couple days ago (2d87f54a6cf435e47c1ea600b48124c7a6873cf8)
So anyway, back to rabbit hole 1!
Equipped with my recent knowledge of
mosaic-schema.json- the rest fell out without too much of battle 😄See changes (compare/main...dangotbanned:mosaic:vgplot-python/fix-dunder-all-2)
I was able to fix
__all__!Findings
I've found some other oddities in the process, which I wanted to mention before opening a PR 3
TL;DR: (1) I unintentionally fixed; (2) I haven't repeated but still can; (3) I have repeated but would prefer not to.
1 -
type->type_?This code is responsible for renaming
type:mosaic/bin/generate-python-api.js
Lines 26 to 30 in 7afc4ec
Which appears here:
mosaic/packages/vgplot/vgplot-python/vgplot/_generated/marks.py
Line 1329 in 7afc4ec
Despite it already having been documented that this is not needed, here:
mosaic/packages/vgplot/spec/src/python-codegen.js
Lines 16 to 20 in 7afc4ec
I only noticed this because I used
keyword.iskeyword4, which correctly allowstypeas akeyword, but that also made me realize there is no coverage for either 😬
2 -
transform-keys.jsThis one confused me, because it lists every transform as they all have "a generated vgplot Python
API function."
mosaic/bin/generate-python-api.js
Lines 212 to 220 in 7afc4ec
To figure out if it did anything at all, I deleted this line where
TRANSFORM_KEYSis used:Show me the diff
I'm very confused by this solution. Nothing about the code
suggests that this is what's happening. Does this make sense to anyone else?
My questions here are:
vg.format?TRANSFORM_KEYSwould be a noopway?
formathelper{"format": {...should be generated by using something namedFORMAT*3 - Docstrings
I hope it isn't too controversial to say that this is a tough read 😉
mosaic/bin/generate-python-api.js
Lines 32 to 43 in 7afc4ec
But the larger issue to me is I don't understand why this is happening in the first place.
The docs in
vgplot/_generated/attributes.pycould be so much better if they just kept thingsas they are in the original (
PlotAttribute.ts)How do I know? Because here's the docs and typing sitting pretty in python!
If you made it this far
Apologies for the essay! 😅
This one leads into why
packages/vgplot/spec-pythoneven exists - which I need to get around to writing more about ⏲️Footnotes
understanding 1 language vs 2 ↩
which I removed in 7e1ab15013e38e2d42c3cb14045dcf535649247a ↩
if there's interest ↩
Python stdlib function ↩