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

Explicitly specify arguments for to_dict and to_json methods for top-level chart objects #3073

Merged
merged 6 commits into from
Jun 2, 2023

Conversation

binste
Copy link
Contributor

@binste binste commented May 29, 2023

This PR explicitly specifies and documents the accepted arguments for to_dict and to_json on top-level chart objects (e.g. Chart, LayerChart, ...). Previously, these methods used args and kwargs. This improves the documentation for users, allows us to type hint the arguments, and it unblocks #3071 (comment) where it is currently unclear how to best add an additional documented keyword argument format.

This might break existing code in case a user passes an unused argument, e.g. to_dict(this_argument_is_not_used=1). However, that seems acceptable to me as I don't think it was the intended behavior to allow for arbitrary arguments.

cc @jonmmease.
Maybe someone else could review it as well. Just in case I'm missing an actual use case for the args/kwargs notation

Copy link
Contributor

@jonmmease jonmmease left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! One suggestion inline for the argument order of to_json.

ignore: Optional[List[str]] = None,
context: Optional[Dict[str, Any]] = None,
indent: int = 2,
sort_keys: bool = True,
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're changing signatures, how would you feel about moving indent and sort_keys above ignore and context. Like this:

    def to_json(
        validate: bool = True,
        indent: int = 2,
        sort_keys: bool = True,
        ignore: Optional[List[str]] = None,
        context: Optional[Dict[str, Any]] = None,

I doubt this would be breaking in practice as ignore and context aren't intended for use by end users, and even if they are being used it would be even more unusual to specify validate ignore and context as positional args.

This is top of mind because I'd rather add the format argument in #3071 before ignore and context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Changed in the latest commit.

@binste
Copy link
Contributor Author

binste commented May 30, 2023

@mattijn @joelostblom Could someone of you review this PR as well just to be sure not to break anything? No pressure at all, just a gentle ping to make sure you see this PR :)

@mattijn
Copy link
Contributor

mattijn commented May 31, 2023

Thanks for the reminder, end of week or early next week I've time.

@mattijn mattijn merged commit 6bd38c2 into vega:master Jun 2, 2023
@mattijn
Copy link
Contributor

mattijn commented Jun 2, 2023

Thanks @binste! Looks good to me too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants