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

Improve developer experience (DX) by improving documentation/annotation and playing nice with static analysis language servers #676

Open
HandcartCactus opened this issue Dec 17, 2024 · 2 comments · May be fixed by #677

Comments

@HandcartCactus
Copy link

Describe the issue

I have used canvasapi and it seems like a great project. I've noticed some DX issues that I'm offering to fix, if you'll let me.

Type Blindness Issues with PaginatedList

Demonstration

Consider the following code:

from canvasapi import Canvas

canvas = Canvas('base url', 'access token')
for course in canvas.get_courses(enrollment_status='active'):
    print(course)

Method and attribute discoverability are hampered by the type-blindness of PaginatedList. Here, it is evident my static analysis language server (pylance) only knows that Canvas(...).get_courses() returns a PaginatedList...
image
...but does not know the elements of PaginatedList are canvasapi.course.Course, as the documentation states.
image
This makes trying to figure out what is possible with course a minor nuisance, and requires looking at the documentation.
image

Suggestions

I do not presume to know how best to fix this issue, but I believe annotating PaginatedList with a TypeVar would fix the issue and provide better support for static analysis servers. It would look something like this:

from typing import TypeVar, Iterable

T = TypeVar('T')

class PaginatedList(object):
    def __init__(
        self,
        content_class:T,
        requester,
        request_method,
        first_url,
        extra_attribs=None,
        _root=None,
        _url_override=None,
        **kwargs
    ):
        ...
    
    def __iter__(self) -> Iterator[T]:
        ...

It might even be helpful to:

class PaginatedList(Iterable[T]):
    ...

An example of this in the wild would be Tableau Server Client's Pager, which also de-paginates server responses.

Attribute Blindness

While documentation doesn't appear to have any issues with displaying available methods, it does appear to hide available attributes, which meant I was constantly using print(vars(...)) to figure out what attributes are available. Take, for example, the documentation for File, which does not mention the attributes 'category', 'content-type', 'created_at', 'created_at_date', 'display_name', 'filename', 'folder_id', 'hidden', 'hidden_for_user', 'id', 'lock_at', 'locked', 'locked_for_user', 'media_entry_id', 'mime_class', 'modified_at', 'modified_at_date', 'size', 'thumbnail_url', 'unlock_at', 'updated_at', 'updated_at_date', 'upload_status', 'url', 'uuid' and 'visibility_level'. It would be great if these were listed!

A static analysis tool also does not see these attributes:
image

The root cause appears to be that the underlying CanvasObject appears to be dynamically loading attributes. Having tried to maintain a python REST API wrapper myself, I understand what a pain it is to have to keep object models up to date, so I am sympathetic. However, Python already has a neat pattern for objects with dynamic attributes, though. It's called a dict! It supports syntactic sugar that CanvasObject does not. It's not a crime to have behavior-based classes, and behavior-based classes are allowed to have attributes, but how are you going to know if you want to .download() a File if it's not even clear that File has a display_name or a filename?

With 68 contributors and tooling like pydantic, swagger, and JSONSchema, it seems like it should be feasible to keep or even generate actual models of the Canvas API responses.

Take, for example, UVA's Canvas API documentation, which has example objects and defined models. Instructure even keeps a model change log.

Even API clients for massive applications such as Reddit have at least attribute lists in the documentation.

This is me officially volunteering to take a swing at at least a few objects and figure out how to maintain such a thing sustainably.

@bennettscience
Copy link
Contributor

Many years, ago, I had opened #363 to explore typing. At the time, it wasn't flexible enough to handle a lot of the issues you're raising here, particularly how to handle **kwargs in functions calls. Any parameter outlined on any endpoint in the Canvas API docs can be included as a keyword argument, but how to maintain that enormous list becomes problematic. The discussion in that thread may be helpful for background.

Typing the PaginatedList object was also brought up in #656, so you can see one approach that was tried. See also the docstring changes made to PaginatedList on #654 which also addresses some of these issues.

You can clone the repo and open a PR for anything you'd like, but given the scope of this, I would start small. Get PaginatedList typing flexibly so that it is more responsive in the LSP/type checker. Make sure to follow the contribution guidelines to avoid issues when you open the PR.

Reclassifying as an enhancement rather than a bug.

@HandcartCactus
Copy link
Author

Great news! Looks like I was able to make PaginatedList type-aware with only a few annotations. Tests are passing.

image
image
image

@HandcartCactus HandcartCactus linked a pull request Dec 18, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants