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

Refactor the function definitions to better support type hints and auto-completion #2896

Open
seisman opened this issue Dec 19, 2023 · 0 comments
Labels
discussions Need more discussion before taking further actions

Comments

@seisman
Copy link
Member

seisman commented Dec 19, 2023

Current style of function definitions

Currently, most of our functions/methods are defined like this:

@use_alias(                                                                     
    C="per_column",                                                             
    I="spacing",                                                                
    T="nearest_multiple",                                                       
    V="verbose",                                                                
    a="aspatial",                                                               
    f="coltypes",                                                               
    i="incols",                                                                 
    r="registration",                                                           
)                                                                               
def info(data, **kwargs):  
    ...

in which **kwargs can be any parameter name, and PyGMT will convert the recognized long-form aliases (e.g., verbose) to the short-form flags (e.g., -V). Using **kwargs makes the function definition short and easy to maintain.

Auto-completion issues

Using **kwargs in function definitions makes it difficult to support tab auto-completion in text editors/IDEs/Notebooks, as reported in #1203.

In issue #1203, I initially proposed to refactor the function definition to something like below so that PyGMT can provide tab auto-completion in any text editors/IDEs/Notebooks.

@use_alias(
    C="per_column",
    I="spacing",
    T="nearest_multiple",
    V="verbose",
    a="aspatial",
    f="coltypes",
    r="registration",
)
def info(
    table,
    per_column=False,
    spacing=None,
    nearest_multiple=None,
    verbose=None,
    aspatial=None,
    coltypes=None,
    registration=None,
    **kwargs
):

The above proposal was rejected (#1203 (comment)) because it means we have to duplicate the aliases in three places (the use_alias decorator, the function definition, and the docstrings). Then, in PR #1282, @maxrjones improved the use_alias decorator to automatically insert the long-form aliases to the function's __signature__ (#1203 (comment)), which makes it possible to support auto-completion in Jupyter Notebooks and IPython.

However, the implementation in PR #1282 has some known limitations:

  1. Auto-completion doesn't work in VSCode (Support tab completion in Jupyter by inserting aliases into the method signature #1282 (comment)) [I don't know the details, but I guess it's because, since __signature__ is dynamically updated, so a Python language server is needed. Jupyter Notebooks has a Python "kernel" but VSCode or other text editors are just an editor).
  2. All parameters have a default value None (Support tab completion in Jupyter by inserting aliases into the method signature #1282 (comment)). It's usually OK, but sometimes a default value False makes more sense. For example, per_column=False is more readable than per_column=None.

Auto-completion support in non-Notebook environments

To support auto-completion in environments like VSCode/IDEs, we need to add type hints (#2794). PEP 692 provides a new way to annotate **kwargs. Following the PEP 692, the function definition will be:

class InfoKwargs(TypedDict):
    # this is just a very simplified version of type hints for **kwargs
    per_column: str | bool = False
    spacing: Any = None 
    nearest_multiple: Any = None
    verbose: str | None = None
    spatial: Any = None
    coltypes: Any = None
    registration: Any = None

@use_alias(
    C="per_column",
    I="spacing",
    T="nearest_multiple",
    V="verbose",
    a="aspatial",
    f="coltypes",
    r="registration",
)
def info(table, **kwargs: Unpack[InfoKwargs]):

As shown in the first animation in PR #2793, after the above refactors, VSCode can auto-complete all the long-form aliases.

We should also be aware of the limitations of the above function definition:

  1. Need to define a TypedDict class like InfoKwargs, which is only used for type hints.
  2. Need to duplicate the aliases in three places (InfoKwargs, the use_alias decorator, and the docstrings)
  3. Doesn't work in Jupyter Notebooks/IPython (POC: Showcases for support type hints #2793 (comment)), so it means we still need the trick in PR Support tab completion in Jupyter by inserting aliases into the method signature #1282

Proposed better solution

I think my old proposal in #1203 is the best solution. The new function definition will be:

@use_alias(
    C="per_column",
    I="spacing",
    T="nearest_multiple",
    V="verbose",
    a="aspatial",
    f="coltypes",
    r="registration",
)
def info(
    table,
    per_column = False,
    spacing = None,
    nearest_multiple = None,
    verbose = None,
    spatial = None,
    coltypes = None,
    registration = None,
    **kwargs,
):

Pros:

  1. Support auto-completion in Notebooks/IDEs/text editors, then we can get rid of the trick in Support tab completion in Jupyter by inserting aliases into the method signature #1282
  2. Likely faster PyGMT, because we no longer need to dynamically update the __signatures__ variable everytime a function/method is called.
  3. Can be extended to support type hints better (no need to define TypedDict classes like InfoKwargs()), e.g.,
@use_alias(
    C="per_column",
    I="spacing",
    T="nearest_multiple",
    V="verbose",
    a="aspatial",
    f="coltypes",
    r="registration",
)
def info(
    table,
    per_column: str | bool = False,
    spacing: Any = None,
    nearest_multiple: Any = None,
    verbose: str | None = None,
    spatial: Any = None,
    coltypes: Any = None,
    registration: Any = None,
    **kwargs,
):

Cons:

  1. Still need to duplicate aliases in three places
  2. Most parameters are not used, so ruff's ARG rules will raise warnings about unused parameters
@seisman seisman added the discussions Need more discussion before taking further actions label Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussions Need more discussion before taking further actions
Projects
None yet
Development

No branches or pull requests

1 participant