Make signatures and docstrings easy to read again

[joelostblom]

I think it is important that our docstrings are easy to read, so that it is easy to quickly look up help without going to the documentation site. Up until altair 5.1.2, the signatures were easy to parse for a human:

Although it is unclear what all the Undefined means above, I get a clear, succinct overview of which arguments exist. Since v 5.2, the signatures are now notably harder to read and directly intimidating at first glance:

The story is similar for the docstring (although less severe), e.g. it used to be relatively easy to understand what the sort parameter does:

But now that section is it is non-really understandable:

I think this stems mainly from #3208 so tagging @binste @mattijn @jonmmease since we were all part of the discussion there. Does anyone have ideas of how we can keep the docstrings human readable while keeping the benefits from all the work @binste has put in on making altair properly typed? There are at least two angles here: the formatting of the docstring (which is mostly up to the IDE), and the content (which is entirely up to us). I would be in favor of keeping the old signatures in the help popup if possible; I'm unsure about what is best in the docstring itself.

I only included screenshots from jupyterlab above, but the situation seems to be similar in vscode:

Our online documentation retains the old easy to read signatures (I'm not sure if we added an exception or just haven't regenerated it since 5.2):

Although the docstring for the parameters has updated to the new more verbose syntax:

[binste]

I agree that the type hints on the autogenerated Altair classes are too verbose right now and that its leaning too strongly on the "have fully specified type hints" side neglecting somewhat the UX. In the docs, I specifically disabled the type hints from showing up in the class signatures - as shown in your screenshot - but I don't know of a way to do this for IDEs as well.

An advantage to the previous state without type hints is, that before one saw all arguments but it was difficult to know what actually needs to be provided. Maybe we can achieve some version in between where we show the most important type hints, in both the docstrings as well as using it in the definitions as well. For example we could not show all specific classes but instead use SchemaBase from which all autogenerated classes inherit from so that type checkers let it pass? As an example, in the definition of Color.aggregate, we could replace Aggregate, ArgmaxDef, ArgminDef, NonArgAggregateOp, and the different Dict[required[argmax]]... etc. with just dict and SchemaBase. Same for condition:

I think the literals are very useful as they provide documentation as well as autocomplete suggestions.

What do you think? Happy to create a PR with these changes so we can play around with how it looks and further take it from there.

[joelostblom]

Thanks @binste, your screenshot is a clear improvement to me in terms of readability. I'm happy to review and iterate on a PR! A few comments on your improvements I can see in the screenshot and the comments you made:

1. Removing the leading Union[]. Improvement without any drawbacks as far as I can see.
2. Removing the redundant leading altair.vegalite.v5.schema.core. Improvement without any drawbacks as far as I can see.
3. Grouping individual classes (such as RepeatRef) into SchemaBase. Here we actually lose some information, which in some cases (such as RepeatRef) provides value, but in many cases (such as ConditionalPredicateValueDefGradientstringnullExpRef) is just noise. Overall, this tradeoff seems worth it to me in terms of making the signature readable. Would all the classes show up in the docstring or would they be grouped as SchemaBase there as well?
4. Related to the above, could we also group UndefinedType under SchemaBase? This seems redundant to me to include in the docstring since it is accepted everywhere. The same goes for indicated that the default value is = UndefinedType in my opinion. I'm not sure what would be a good replacement, but I'm leaning towards just removing it that doesn't break anything, since many of the parameters actually have default values that are detailed in the docstring and indicating that the parameters are undefined is not really correct anyways.
5. Regarding keeping the string literals, I agree with you. I wish the IDE would indent one level instead of de-indent one level when it is wrapping long strings over multiple lines. This would make it much easier to read, maybe I will open an issue about that for Jupyterlab and VScode.

[binste]

Thanks for the nice write up!

1. did not change, the difference is only because my screenshot is from VS Code and I think you compared it against JupyterLab. Is that possible?
2. Same as 1.
3. For docstrings, we can choose. I'd suggest to also use SchemaBase to keep it simple. What do you think?
4. I do see that it is repetitiv. However, type hints aside for a moment, Undefined is the actual default value of those arguments in Python and so we have to keep it as a default value. Vega-Lite then interprets this Undefined as "fall back to default value" but this default value is set in Vega-Lite -> The VL spec produces by Altair won't contain such an element. As we can't get rid of Undefined, we also have to have a type hint for it. UndefinedType is not a subtype of SchemaBase right now and apart from the type hints, I don't think it makes sense to change that as it does not support any of the methods defined on SchemaBase.
5. Thanks for opening the two issues! Would be great if the display in VS Code and Jupyterlab can be improved :)

I'll request your review on the PR once I have it ready.

[joelostblom]

1. Yup, you're correct. I opened Remove Union and API prefixes in help pop up signatures and docstrings jupyterlab/jupyterlab#15606
2. same
3. I'm torn, it is useful to see RepeatRef but not as much for the longer more obscure classes. I'm probably leaning slightly towards having the full description in the docstring and the simplification only in the signature, but if it is notably easier to keep it the same and use SchemaBase for both, I'm happy with that too.
4. I see, so there is no way to get rid of the undefined default value... Since we have it everywhere, I wish we could just leave it out and write a single session in the docs saying that all default values are undefined in Altair and set by Vega-Lite. Maybe we can even find the default values from the Vega-Lite API somehow? That's probably a bigger discussion; I can open a separate issue for it (or find the old one where we discussed Python vs VL types).

[binste]

Re 1., that's great! I hope they are onboard with the suggestion.
Re 3. It's easy to have the full description in the docstring so happy to implement that as well.
Re 4., if we'd replace Undefined with default values as Vega-Lite uses them (assuming we can infer this), it would blow up the VL specification that Altair produces as all properties would then be included. So I don't think there is a way around having Undefined as default value and UndefinedType as type hint :)

[joelostblom]

Thank you for updating the PR, I will have a look!

Re 4, I was thinking of only including the defaults in the docstring/signature but not in the actual spec that is produced; maybe that's impossible/confusing. And I think they are already mentioned in the docstring text, so it's probably clear enough that Undefined actually becomes a value.

Another option would be to use None instead of a custom undefined type. Then I think the signature would be less verbose as I believe only None shows up and not something like NoneType = None, is that correct? I'm hoping for something like this:

However, it seems like it will be hard to use None as when I was looking into why we had Undefined instead of None, Jake had mentioned it is because None translates to null in json which has a special meaning in VegaLite. So it would be impossible to distinguish a None that means "default undefined value" and a None that means "explicit null". At the same time I do agree that seeing "Undefined" everywhere is somewhat confusing because the chart that is produced clearly does not have all these properties as undefined even if they technically are undefined at the altair level.

[binste]

Good point, we could filter out the default values again from the spec although we could then not differentiate if someone intentionally sets a value and it just happens to be the default value or not. This might also not matter. I don't know if any of this is possible but interesting to think about :)

None also requires a type hint which again is None (the type would be NoneType but type checkers accept None as well). In the older type hint notation you would use Optional -> bandPosition: float | None = None or bandPosition: Optional[float] = None.