-
Notifications
You must be signed in to change notification settings - Fork 789
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
feat: Generate expr
method signatures, docs
#3600
base: main
Are you sure you want to change the base?
Conversation
Similar idea to `inspect.Parameter`
Similar to `inspect.Signature`
- Only the most common case - no docs - No method body
Currently just collects the pieces, but doesn't render the final str
Also renamed constant `EXPR_ANNOTATION` -> `INPUT_ANNOTATION`
- Contains all the functionality - Needs a lot of tidying up
- Uses the same config as `indent_docstring` - Can't truly be `numpydoc` though without a parameters section
Related to (but doesn't resolve) #3600 (comment)
There were 10+ cases of methods not being defined. These are now all present #3600 (comment)
@mattijn following your feedback in #3536 (comment) I'm trying to write a helpful description but having a hard time, without either:
This is otherwise ready for review, so I wanted to ask ahead to see what content you'd like to see there |
expr
method signaturesexpr
method signatures, docs
Thanks for requesting a review. You can help me explaining how I can review this. Do I have to run |
@mattijn
The actual code generation you could call yourself, but this is already in the CI. |
I get proper signatures when doing import altair as alt
alt.expr.scale()
But not when doing: import altair.expr as ae
ae.scale()
Is that something that is expected? Or was it naive to expect that something as such could work? |
@mattijn I managed to get all of these working correctly: # ruff: noqa: B018
import altair as alt
# import altair.expr as ae
from altair import expr
from altair import expr as ae
from altair.expr import expr as expr_cls
alt.expr.scale
expr.scale
ae.scale
expr_cls.scale EditAFAIK this behavior isn't something I'd expect to have changed vs |
# NOTE: `altair` types (for annotations) | ||
RETURN_WRAPPER: LiteralString = "FunctionExpression" | ||
RETURN_ANNOTATION: LiteralString = "Expression" | ||
""" | ||
The annotation is intentionally *less* specific than the real type. | ||
|
||
``Expression`` is shorter, while preserving all the user-facing functionality | ||
""" | ||
|
||
CONST_WRAPPER: LiteralString = "ConstExpression" | ||
CLS_META: LiteralString = "_ExprMeta" | ||
INPUT_ANNOTATION: LiteralString = "IntoExpression" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
Adding this comment to provide a link from #3616
tools/schemapi/vega_expr.py
Outdated
class Source(str, enum.Enum): | ||
"""Enumerations for ``expressions.md`` source files.""" | ||
|
||
LIVE = "https://raw.githubusercontent.com/vega/vega/main/docs/docs/expressions.md" | ||
STATIC = "https://raw.githubusercontent.com/vega/vega/fb2e60274071033b4c427410ef43375b6f314cf2/docs/docs/expressions.md" | ||
OLD = "https://raw.githubusercontent.com/vega/vega/ff98519cce32b776a98d01dd982467d76fc9ee34/docs/docs/expressions.md" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
- Use release versions instead of commit hashes
- Investigate linking the
vega
version toaltair/tools/generate_schema_wrapper.py
Line 49 in cabf1e6
SCHEMA_VERSION: Final = "v5.20.1"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided a manual solution in d75792b
(#3600)
So far I've only come up with this as a source for automating:
@mattijn I guess I have two questions:
- Would manually syncing the
(vega|vega-lite)
versions seem reasonable to you? - Do you know of any simpler sources for getting the corresponding
vega
minimum version for a givenvega-lite
release?
I'm unsure whether using package.json
is a good/bad idea, it was just the first source I found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tentative solution in:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manually syncing is fine by me. Adding a note in this section that we should update the VEGA_VERSION
by eg inspecting the file you found or just the latest released Vega version.
Well spotted @mattijn thank you! I'll follow this up in #3600 (comment) |
Not fully convinced this is a reliable alterntive to manually maintaining #3600 (comment)
Closes #3563
Description
This PR builds on the work of #3466, and provides accurate signatures for
alt.expr
methods.Benefits
Originally listed in #3563 (comment)
TypeError
at the time ofexpr
definitionChartType
python
and notjavascript
expr
and the missing argument""
for theelseValue
Screenshots
Implementation
In #3563 I proposed authoring these changes manually.
However, after revisiting a conversation I had with @jonmmease I thought it would be worth at least trying to generate the definitions.
I found that https://vega.github.io/vega/docs/expressions/ seemed to be the only place they are documented in a single place.
From my understanding, this document is manually maintained but updates occur infrequently.
There was enough consistency in the markup used to work out how various parameters aligned with
python
syntax https://docs.python.org/3/tutorial/controlflow.html#special-parameters.I also used this opportunity to clean up the docs themselves, and get them closer to the rest of
altair
.There is definitely more that could be explored on that front, and some of the changes here could be utilised to improve the existing generated docs.
Related
expr
as a class that is understood by IDEs #3466Tasks
py
-equivalent signaturesaltair
functionsbackticks
for own parametersmistune
escapingtest_expr.py
to account for number of arguments relatedalt.expr.__init__.py
.rst
stuff commitvega_expr.py
expr
method signatures, docs #3600 (review)Deferred
Trying my best to keep the scope of the PR focused.
Working on this sparked a lot of ideas, so I've collected those here to possibly explore in the future:
expressions.md
tools.schemapi.vega_expr.py
->tools.vega_expr.py
vega-lite
schemaexpr
methods, replace w/ snake_case_ExprMeta.__getattr__
would make this possiblePreview
Prior to
5e75051
(#3600) there wasn't any generated source on this branch.Keeping these screenshots for a quick reference
until I've resolved some test issues.Screenshots