feat: Reimplement `expr` as a class that is understood by IDEs by dangotbanned · Pull Request #3466 · vega/altair

dangotbanned · 2024-07-09T17:48:44Z

Related #2904

`expr`

all ConstExpressions are now read-only properties stored in a metaclass
expr subclasses ExprRef, replacing the __call__ w/ __new__
- No instances of expr are ever created
All FunctionExpressions are now classmethods
Each Expression can now be referenced in autocomplete, without needing IPython/Jupyter

Docs

Docstrings have been reformatted to be compatible with sphinx
Broken links in docstrings fixed (e.g. for d3)
class-level doc for expr, with references & doctest
Added to API Reference
- uses the section heading proposed in feat: Improve the syntax for conditions with multiple predicates #3427 (comment)

Demo

Current (IPython)

2024-06-20.13-30-20.Current.IPython.mp4

Current (Static)

2024-06-20.13-31-45.-.Current.Introspection.mp4

Proposed (Static)

2024-07-09.18-08-29.-.Improved.Introspection.mp4

Proposed (API Reference)

Fixes the mypy issue referenced in `alt.__init__.py`

The results of `_populate_namespace` were only visible in a `IPython` environment

…ngleton - all `ConstExpression`s are now read-only properties stored in a metaclass - `expr` subclasses `ExprRef`, replacing the `__call__` w/ `__new__` - No instances of `expr` are ever created - All `FunctionExpression`s are now classmethods - Each `Expression` can now be referenced in autocomplete, without needing `IPython`/`Jupyter` - Docstrings have been reformatted to be compatible with `sphinx` - Broken links in docstrings fixed (e.g. for d3) - class-level doc for `expr`, with references & doctest

Previously relied on the dynamic globals

Previously 2 tests, now 170+

Uses the section heading proposed in vega#3427 (comment) Expands on vega#2904

dangotbanned · 2024-07-09T18:18:39Z

All tests are passing now 😀

I hope the recordings/screenshots are useful. I thought demonstrating the UX would be clearer than listing all the benefits this could have.

Happy to clarify anything if you have any questions

altair/expr/__init__.py

jonmmease · 2024-07-15T18:53:08Z

Hi @dangotbanned, thanks for this PR and apologies that no one has gotten back to you on it yet. This looks like a really nice quality of life improvement, and is very in line with my desire to promote the use of the expression API over using expression strings.

I'll try to get to reviewing and playing with this later in the week if no one else gets to it faster

I left this out initially, but by adding static `Parameter` names it is now possible

binste · 2024-07-16T15:59:07Z

Hi @dangotbanned, thanks for this PR and apologies that no one has gotten back to you on it yet. This looks like a really nice quality of life improvement, and is very in line with my desire to promote the use of the expression API over using expression strings.

I'll try to get to reviewing and playing with this later in the week if no one else gets to it faster

Can only second Jon's statement. Looks like a big UX improvement 🥳 but I struggle to find the time to review it. If you Jon get to it, that would be awesome! Thank you both :)

These will all be superseded by vega#3466

https://github.com/vega/altair/actions/runs/10005909777/job/27657526013?pr=3466

jonmmease · 2024-07-19T14:07:21Z

This is an awesome improvement @dangotbanned. I like the architecture, just wondering if you have thoughts on reducing the duplication of function/constant specifications between the new class methods and the current FUNCTION_LISTING and CONST_LISTING dictionaries. I think the latter are only referenced in tests now, and it would be nice to remove them so that we don't risk them getting out of sync with the class methods.

Since we don't autogenerate these definitions right now, I want to go through the functions one-by-one before merging, and I'll do this later today or over the weekend.

Not to address in this PR, but it would be nice to eventually be able to auto-generate these function definitions, but it looks like they aren't included in the Vega or Vega-Lite jsonschema definitions, so we would need to pull them directly from the Vega repo or something.

dangotbanned · 2024-07-19T14:42:30Z

Thanks @jonmmease

This is an awesome improvement @dangotbanned. I like the architecture, just wondering if you have thoughts on reducing the duplication of function/constant specifications between the new class methods and the current FUNCTION_LISTING and CONST_LISTING dictionaries. I think the latter are only referenced in tests now, and it would be nice to remove them so that we don't risk them getting out of sync with the class methods.

Yeah I considered this and would +1 for removing them.
I was trying to make this a drop-in replacement, as I imagined it would be easier to get merged.

The related changes I'd like to see here are:

Remove expr.[funcs|consts].py modules entirely
~~Rewrite the tests to iterate over wherever @classmethods are stored~~ 65b527a (#3466)

Since we don't autogenerate these definitions right now, I want to go through the functions one-by-one before merging, and I'll do this later today or over the weekend.

Good luck! There is a lot to read.
I think some were out of sync with the current descriptions, so I updated those that I spotted.
You may also notice markup changes I had to make - to keep sphinx happy - which would be a consideration if this were autogenerated.
Although IIRC tools/.. has some handling for this already, so maybe not an issue.

Not to address in this PR, but it would be nice to eventually be able to auto-generate these function definitions, but it looks like they aren't included in the Vega or Vega-Lite jsonschema definitions, so we would need to pull them directly from the Vega repo or something.

Yeah I looked into that as well and I couldn't see a clean way of doing so, but maybe someone more familiar with vega/vega-lite could?

They're all defined in one place here https://vega.github.io/vega/docs/expressions/

But the actual source seems to be files like:

Fixes vega#3466 (comment)

dangotbanned · 2024-07-19T17:27:00Z

This is an awesome improvement @dangotbanned. I like the architecture, just wondering if you have thoughts on reducing the duplication of function/constant specifications between the new class methods and the current FUNCTION_LISTING and CONST_LISTING dictionaries. I think the latter are only referenced in tests now, and it would be nice to remove them so that we don't risk them getting out of sync with the class methods.

@jonmmease all done on that front 👍

I'll hold of on my suggestion to remove, but this can now be done at any point in the future

@dangotbanned

The related changes I'd like to see here are:

Remove expr.[funcs|consts].py modules entirely

jonmmease

lgtm!

dangotbanned · 2024-07-20T13:18:28Z

lgtm!

Thanks @jonmmease

FYI I'm working on a fix for the mypy issue that appeared on merging - will be in a new PR

jonmmease · 2024-07-20T13:30:42Z

FYI I'm working on a fix for the mypy issue that appeared on merging

Thanks!

dangotbanned · 2024-07-20T16:16:05Z

FYI I'm working on a fix for the mypy issue that appeared on merging

Thanks!

@jonmmease

fix(typing): Resolve mypy==1.11.0 issues in plugin_registry #3487

Is ready and would be able safely merge #3485 after

Thank you again for reviewing

Based on vega#3466 (comment) With some additional ignores added during test runs on `/v5/schema/*.py`

dangotbanned added 10 commits July 9, 2024 16:38

fix: Use absolute imports explicitly

0813874

Fixes the mypy issue referenced in `alt.__init__.py`

refactor: remove dynamic globals manipulation

bcb8f1d

The results of `_populate_namespace` were only visible in a `IPython` environment

fix: use absolute imports in test_expr

b634913

Previously relied on the dynamic globals

test(perf): rewrite expr tests to run in parallel

02fccf5

Previously 2 tests, now 170+

test: confirm expr constants stay constant

77a2073

chore(typing): add ignores not flagged by mypy

fe09616

docs: adds alt.expr to API Reference

8a80357

Uses the section heading proposed in vega#3427 (comment) Expands on vega#2904

test: ensure test_expr_consts_immutable pattern works for all versions

2b7a2b3

Merge branch 'main' into expr-non-dynamic

e1182d4

dangotbanned marked this pull request as ready for review July 9, 2024 18:15

docs: fix typos, regex artifacts, apply some ruff D rules

34430a0

dangotbanned commented Jul 10, 2024

View reviewed changes

altair/expr/__init__.py Show resolved Hide resolved

dangotbanned mentioned this pull request Jul 11, 2024

More convenient syntax instead of FieldOneOfPredicate #3239

Open

dangotbanned added 3 commits July 13, 2024 17:28

Merge branch 'main' into expr-non-dynamic

c360965

Merge branch 'main' into expr-non-dynamic

448f1cf

Merge branch 'main' into expr-non-dynamic

4b2e0c2

dangotbanned added 3 commits July 16, 2024 09:09

Merge branch 'main' into expr-non-dynamic

ed6a819

docs: add doc for _ConstExpressionType

a33d075

test: make expr doctest testable

f4e7b65

I left this out initially, but by adding static `Parameter` names it is now possible

joelostblom added the enhancement label Jul 17, 2024

dangotbanned mentioned this pull request Jul 17, 2024

fix: Restrict static & runtime top-level imports #3482

Merged

Merge branch 'main' into expr-non-dynamic

7c29281

dangotbanned added a commit to dangotbanned/altair that referenced this pull request Jul 18, 2024

revert: remove unrelated expr changes

0f54057

These will all be superseded by vega#3466

dangotbanned added 2 commits July 19, 2024 10:34

Merge branch 'main' into expr-non-dynamic

7185238

fix: re-run generate-schema-wrapper

08448d1

https://github.com/vega/altair/actions/runs/10005909777/job/27657526013?pr=3466

dangotbanned added 3 commits July 19, 2024 18:02

refactor: Remove expr test dependency on constants

65b527a

Fixes vega#3466 (comment)

Merge remote-tracking branch 'upstream/main' into expr-non-dynamic

f78c401

style: remove trailing commas in @pytest.mark.parametrize

ece2738

jonmmease approved these changes Jul 20, 2024

View reviewed changes

jonmmease merged commit b9d3fd4 into vega:main Jul 20, 2024

dangotbanned deleted the expr-non-dynamic branch July 21, 2024 11:02

dangotbanned added a commit to dangotbanned/altair that referenced this pull request Jul 22, 2024

ci(ruff): Update rules to use pydocstyle

ec5164e

Based on vega#3466 (comment) With some additional ignores added during test runs on `/v5/schema/*.py`

dangotbanned mentioned this pull request Jul 22, 2024

refactor(docs, ruff): Add pydocstyle rules #3493

Merged

dangotbanned mentioned this pull request Aug 2, 2024

feat(python!): Use Altair in DataFrame.plot pola-rs/polars#17995

Merged

dangotbanned mentioned this pull request Sep 19, 2024

feat: Generate expr method signatures, docs #3600

Merged

19 tasks

dangotbanned mentioned this pull request Oct 3, 2024

Support alt.datum[signal_name] in addition to "datum[signal_name]" #3366

Open

dangotbanned mentioned this pull request Apr 25, 2025

feat(RFC): Adds altair.datasets #3631

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: Reimplement `expr` as a class that is understood by IDEs#3466

feat: Reimplement `expr` as a class that is understood by IDEs#3466
jonmmease merged 23 commits intovega:mainfrom
dangotbanned:expr-non-dynamic

dangotbanned commented Jul 9, 2024 •

edited

Loading

Uh oh!

dangotbanned commented Jul 9, 2024 •

edited

Loading

Uh oh!

Uh oh!

jonmmease commented Jul 15, 2024

Uh oh!

binste commented Jul 16, 2024

Uh oh!

jonmmease commented Jul 19, 2024

Uh oh!

dangotbanned commented Jul 19, 2024 •

edited

Loading

Uh oh!

dangotbanned commented Jul 19, 2024

Uh oh!

jonmmease left a comment

Uh oh!

dangotbanned commented Jul 20, 2024

Uh oh!

jonmmease commented Jul 20, 2024

Uh oh!

dangotbanned commented Jul 20, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

dangotbanned commented Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

expr

Docs

Demo

Current (IPython)

Current (Static)

Proposed (Static)

Proposed (API Reference)

Uh oh!

dangotbanned commented Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jonmmease commented Jul 15, 2024

Uh oh!

binste commented Jul 16, 2024

Uh oh!

jonmmease commented Jul 19, 2024

Uh oh!

dangotbanned commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dangotbanned commented Jul 19, 2024

Uh oh!

jonmmease left a comment

Choose a reason for hiding this comment

Uh oh!

dangotbanned commented Jul 20, 2024

Uh oh!

jonmmease commented Jul 20, 2024

Uh oh!

dangotbanned commented Jul 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dangotbanned commented Jul 9, 2024 •

edited

Loading

`expr`

dangotbanned commented Jul 9, 2024 •

edited

Loading

dangotbanned commented Jul 19, 2024 •

edited

Loading

dangotbanned commented Jul 20, 2024 •

edited

Loading