WIP: Tab completion 2

[ChristopherDavisUCI]

Branch off of #2592 by @jakevdp and @joelostblom. Adds type hints for the setter methods to help with tab completion.

@mattijn Can you please take another look and see if this new version addresses your concern from #2769 (comment)?

I've never used type hints before, so I'm very happy for any comments or corrections!

[mattijn]

Different approach I see!

Tab-completion works for the chained options👏:

For the first chained method the docstring/signaturea appear (using shift+tab in jlab)

But signature won't appear within the second chained method🫣:

Almost there it seems!

Or is this because of a limitation within ipython? See jupyterlab/jupyterlab#12570 (comment).

[mattijn]

When looking to the code change I notice the following:

    @overload
    def aggregate(self, _: str, **kwds) -> 'Angle':
        ...

    @overload
    def aggregate(self, argmax=Undefined, **kwds) -> 'Angle':
        ...

    @overload
    def aggregate(self, argmin=Undefined, **kwds) -> 'Angle':

It seems it defines somehow multiple identical named methods with different arguments combinations.

[ChristopherDavisUCI]

Thanks @mattijn! I didn't know about this @overload decorator until a few hours ago, but my understanding is that it is specific to type hinting (it is defined in the typing module) and it is a way of offering different possible signatures for the same function. For example, calling bin(True) is valid, as is calling bin(extent=[-4, 4], step=0.5), and so this is a way to tell the (linter? type checker?) that both are okay. In the previous PR, I had used Union (also from the typing module), but I think this @overload approach feels less hacky.

Notice that none of these lines you posted are defining anything; they're just indicating what types of arguments are valid.

Does that make sense?

I will think about the signature question (but it might be more a question related to @joelostblom's earlier commits).

[mattijn]

Thanks for the explanation @ChristopherDavisUCI! Will give these changes a few more trials, but nice👍.

[joelostblom]

Or is this because of a limitation within ipython? See jupyterlab/jupyterlab#12570 (comment).

Yes, I think that is the issue and not something we can easily fix on the Altair side of things I believe =/ I'm really quite surprised that this isn't considered a more important issue as method chaining is used so often but maybe it is a really complex problems to solve?

[mattijn]

Thanks @joelostblom, are you aware of a GitHub issue raising this somewhere higher up, like in ipython?
While playing with this PR I missed the docstrings:/.

[joelostblom]

I'm not unfortunately. I wasn't sure what would be a good way to test it in Ipython since the shift+tab shortcut is for JupyterLab only and the tab completion works as expected in both jupyterlab and ipython

[jcmkk3]

I'm not unfortunately. I wasn't sure what would be a good way to test it in Ipython since the shift+tab shortcut is for JupyterLab only and the tab completion works as expected in both jupyterlab and ipython

@joelostblom I tested your example from the jupyterlab issue linked above with xeus-python. I get the same behavior with the ipykernel and XPython kernels, but if I use the XPython Raw kernel (doesn't use ipython in any way), then it works. I think that confirms that it is a ipython/ipykernel issue and not a jupyter issue.

[ChristopherDavisUCI]

Thanks @mattijn, yes, it should be very possible for me to work on this. At least the third one should be no problem, and I'll let you know if I get stuck on 1 or 2.

[binste]

Very exciting to see the progress here, thanks everyone involved!

I like the idea of explaining the new syntax in a separate section in the docs but not modifying the rest of the docs and the examples just yet. What do you think about making this the actual 5.0 release instead of only a release candidate? Once there is enough positive feedback from users the approach could be made the standard one by updating the documentation everywhere and publishing a minor release (there will probably be a few bug fixes and new features until then anyway), e.g. 5.1.

As it's not a backwards-incompatible change, this would be fine following semantic versioning and it would allow us to already publish the new documentation (don't think we would update it for an rc version?). Furthermore, it would be great to get a proper 5.0 release out soon as it will contain quite a few important bug fixes (e.g. #2771 <- fresh Altair v4 installation with recent jsonschema versions will break some charts) and new features (vl-convert-python support, vega-lite 5, etc.).

[ChristopherDavisUCI]

Is "update from upstream/master" a good thing to do before working on the docs? Or is bringing in those 19(?) commits too messy? Should I instead work on the docs in a different branch off of the master branch?

[mattijn]

No is fine, you can update from master. But if you have to undo all commits from this PR regarding the changes to the examples, it might be easier to open a new PR in a new branch?

[ChristopherDavisUCI]

@mattijn I thought your comment 3 about alt.datum would be the easiest to implement, but I think I was over-confident. This alt.datum is defined pretty minimally https://github.com/altair-viz/altair/blob/f288e2df580b8a2059f086dc8f67e2b11e340598/altair/expr/core.py#L4-L23 as something that returns a dictionary when called. For example, type(alt.datum(5)) is dict.

To me, at this point it doesn't seem worth rewriting this alt.datum in a way that the property setters work on it. (In the best case scenario, we would need x=alt.datum to have different methods than color=alt.datum.) If someone is trying to use x=alt.datum(5).scale(...), we can always instruct them to use x=alt.XDatum(5).scale(...) instead.

Does that seem reasonable?

I should have some time today to think about your other comments 1 and 2.

[mattijn]

That is unfortunate. I agree with you that it is not worth too much effort at the moment. Just mention this limitation is enough for now!

[ChristopherDavisUCI]

Thanks @mattijn! I've put the changed examples into an attribute_syntax folder and replaced them with whatever was in the main branch. Is this along the lines of what you had in mind in comment 2?

[mattijn]

👍 perfect

[ChristopherDavisUCI]

Hi @mattijn and @joelostblom, I added some basic documentation in this last commit (briefly explaining the setter methods), and at the end mentioning that it does not work with alt.datum (nor with alt.value). I'm happy for any comments!

@binste I don't have a great overview of the current layout of the docs, so feel free to move this to another location or make changes to make it more inline with what you have.

[binste]

I like the placement in the docs and how it builds on the previously introduced example.

[ChristopherDavisUCI]

Thanks for all the feedback @binste!

@mattijn please let me know if there is anything else here you'd like me to add or change!

[mattijn]

I've modified the tests so the examples using the attribute syntax method are integrated within the tests. All are passing, so that is good. But what I don't like, which becomes much clear in this PR, is that all tests(-files) are included as part of the Altair package. A few test files are fine, but now, there are many. Maybe this is a good moment to extract the test-files from the Altair folder into a specific test folder? Maybe good to do this in a separate PR, but I'm not sure how interwoven the tests are with the Altair package.

Opened a new issue for this: #2788

[mattijn]

Hi @ChristopherDavisUCI! First, best wishes for 2023!
I tried to rebase this PR with master, but I ran into some conflicts. I tried to solve these conflicts, but I was not able to pass commit 0dbbd260d18d229dc349142fc2ef6da6619dcf77 with the error message could not apply 0dbbd260... Lifting parameters to the top level. Which is a bit strange for me, since I there is no lifting of parameters in this PR as far I'm aware. Maybe that part was not yet included when this PR started?

[ChristopherDavisUCI]

Thanks @mattijn, Happy New Year to you too!

Are you able to tell what file is causing the problem? I tried going to Github Desktop and "Update from upstream/master", and about 400 "conflicts" show up, but most of them seem to be related to the examples.

I think you're right that @joelostblom's version of this PR did start before the parameter work.

Worst case scenario I should be able to manually make the changes in this PR starting with the main branch.

I'm very happy for any suggestions of what you think is the best way to proceed!

[mattijn]

Maybe we should create a new branch and git cherry-pick the commits we want from here into this new branch. Then the commit-credits are still tracked to the right contributors. There still will be a bit of refactoring necessary, but at least the history is as much as possible intact.

One thing that I don't know of is the following: how will this upcoming PR: #2732 work with this commit: cc5ca7c? Would it still be necessary to get the docstring of the helper class? Or by setting up aliases the signature will be treated as general descriptors of these parameters? Probably not and is better to test in #2732 once we can merge this first.

[ChristopherDavisUCI]

Thanks @mattijn, I'll take a look at this over the next few days and let you know if I get confused.

[ChristopherDavisUCI]

Hi again @mattijn, I created a new PR at #2795 if you want to keep an eye on it. I'll keep you updated on how it's going. I've only cherry-picked one commit so far.

[ChristopherDavisUCI]

Closing in favor of #2795.