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

feat: expr method signatures #3600

Draft
wants to merge 68 commits into
base: main
Choose a base branch
from
Draft

feat: expr method signatures #3600

wants to merge 68 commits into from

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Sep 19, 2024

Will close #3563

Description

placeholder - fill before leaving draft

Tasks

Deferred

  • Docs
    • Inline code -> block code (comment)
  • Also parsing properties from expressions.md
    • Wouldn't be that much work to add, but not a great deal of value
    • We're missing MAX_VALUE, MIN_VALUE
  • tools.schemapi.vega_expr.py -> tools.vega_expr.py
    • The module is unrelated to the vega-lite schema
    • I think the review will be simpler if it stays here for now
  • Soft-deprecating camelCase expr methods, replace w/ snake_case
    • Pretty sure adding an _ExprMeta.__getattr__ would make this possible
    • The current names would remain accessible (but undocumented)

Related

Preview

Prior to 5e75051 (#3600) there wasn't any generated source on this branch.
Keeping these screenshots for a quick referenceuntil I've resolved some test issues.

Screenshots

image

image

image

- 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
Comment on lines +1052 to +1057
The optional ``specifiers`` object provides a set of specifier sub-strings for customizing
the format; for more, see the `timeUnitSpecifier API documentation`_. The resulting
specifier string can then be used as input to the `timeFormat`_ or `utcFormat`_ functions,
or as the *format* parameter of an axis or legend. For example: ``alt.expr.timeFormat(date,
alt.expr.timeUnitSpecifier('year'))`` or ``alt.expr.timeFormat(date,
alt.expr.timeUnitSpecifier(['hours', 'minutes']))``.
Copy link
Member Author

@dangotbanned dangotbanned Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline -> block code idea

An example of how this change would show

def func(*args):
    """
    ...
    The optional ``specifiers`` object provides a set of specifier sub-strings for customizing
    the format; for more, see the `timeUnitSpecifier API documentation`_. The resulting
    specifier string can then be used as input to the `timeFormat`_ or `utcFormat`_ functions,
    or as the *format* parameter of an axis or legend. 

    Examples
    --------
    >>> alt.expr.timeFormat(date, alt.expr.timeUnitSpecifier('year'))
    >>> alt.expr.timeFormat(date, alt.expr.timeUnitSpecifier(['hours', 'minutes']))
    """
    ...

Issues

  • Some of these uses the pattern "... -> result"
    • This can be recreated using the doctest syntax
    • but the actual result won't be the same as what is stated, since they are not evaluated
  • alt.expr.format uses a different style
    • "(e.g., alt.expr.format(value, ',.2f')"
    • Whereas all the others use ("For example:..." | "Example:...")

Also some rearranging in `vega_expr.py`
Comment on lines 198 to 219
def _doc_fmt(doc: str, /) -> str:
"""
FIXME: Currently doing too many things.

Primarily using to exclude summary line + references from ``textwrap``.
"""
sentences: deque[str] = deque(SENTENCE_BREAK.split(doc))
if len(sentences) > 1:
references: str = ""
summary = f"{sentences.popleft()}.\n"
last_line = sentences.pop().strip()
sentences = deque(f"{s}. " for s in sentences)
if "\n\n.. _" in last_line:
last_line, references = last_line.split("\n\n", maxsplit=1)
sentences.append(last_line)
sentences = deque(text_wrap.wrap("".join(sentences)))
sentences.appendleft(summary)
if references:
sentences.extend(("", indent(references, 8 * " ")))
return "\n".join(sentences)
else:
return sentences.pop().strip()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issues

  • Very tightly coupled to how RSTRenderer appends links
  • Don't think re.split is that intuitive, despite working for this specfic case
  • Would prefer something more structured

- Moves a replacement that previously occured in two places (rendering & signatuyre parsing)
- Now the tokens returned by `read_ast_tokens` do not contain this at all

#3600 (comment)
Step is unrelated to parsed attributes of a definition
This method was always intended to be temorary and is no longer helpful
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve expr method signatures
1 participant