Code Review & Improvement Plan

[jakobkogler]

As you noticed in #174, I dived into the codebase a bit.
The main reason is, that I'm a bit unhappy with the architecture and usability of the library.
(But don't get me wrong, I'm also very happy that this library exists in the first place.)

I'll list a couple of possible improvements. Please tell me if you are willing to incorporate them into this repo.
If so, I'm happy to provide a couple of PRs in the near future.

1. User Experience

1a. Typing

There is currently almost no typing support for this library. Which makes it really, really hard to use.
Basically every single return value is just a dict. And you don't know what's inside it.
You basically need to experiment with the library, run it, check the output, read the code, ...
And in some cases there is typing, but it's just wrong.
I had so many bugs (in my own project) because of that. Examples:

• In some movie objects like the search results you can access the directors with movie["directors"] (notice plural), in some it doesn't even exist (like for the Movie object inside a review search result), and in some other objects you need to access them with movie.crew["director"] (notice singular).
• The year property of a Movie is typed as int. Great. But you can't rely on it, because in special cases (e.g. unreleased or very rare movies) there doesn't exist a year and the value is None.
• Parsing the response of the Search class is horrible. First you need to understand the Pagination dict around them, and then you see that there are dicts of different shapes inside it (review vs user vs list vs movie, ...) and you need to distinguish them via hardcoded strings...
• User(....).get_films()["movies"] returns a dictionary and not a list. It's a dictionary that has slugs as keys (why???)

Currently if want to know if the year is always an int (e.g. because you want to do a comparison like movie.year > 2000), you would actually need to read the code and follow 5 function calls until you find the code that confirms that it can be sometimes None.
That's almost the same work as just parsing the letterboxd website on your own without a library.

If the code is typed (which should be the default for a parsing library) and especially correctly typed, all those problems will go away.

=> So I suggest to excessively use of dataclass instead of dict. And use a type checker like pyrefly or ty to check that the annotated types are correct. E.g. the result of a search could be a dataclass called MovieRef (reference to a movie, since it only contains limited data).
And when somebody uses the library the IDE can then autocomplete a code like movie_ref.dire and highlight errors like movie_ref.year > 2000.

(Btw, in order to avoid breaking changes, the dataclasses could have a __getitem__ method with a warning so that the old way still works.)

1b. Inconsistent behaviors / non-intuitive behaviors

I mentioned some of the non-intuitive / inconsistencies already in the previous section. E.g. the name "movie" vs. "film" in User("username").get_films()["movies"] (Btw Letterboxd only uses the name "film", never "movie") or the movie["directors"] vs. movie.crew["director"] case.

Some bigger ones though:

Movie("v-for-vendetta") makes a request (or multiple). That's really surprising, as CTORs are not allowed to do that in most languages. In Python it's somewhat relaxed (it's not even a CTOR), but I would still call it an anti-pattern.
Also at the same time some other classes like Search("V for Vendetta") don't make request. Quite confusing.

=> I would not allow constructing a Movie object, and force users to use some class methods like Movie.fetch_by_slug(...) and Movie.fetch_by_imdb(...). Notice also the suggestion fetch_by instead of just from_slug, to indicate to the user that there is actually a request and that it's not a free function call.

=> And instead of the search object (which has no real benefit anyway, as it just temporarily stores the search parameters), break it into a bunch of nice functions: def search_film(search_term: str, max_results: int = 10) -> SearchResult[MovieRef]: and def search_all(search_term: str, max_results: int = 10) -> SearchResult[MovieRef | ListRef | UserRef | ... ]: with a response class like this:

T = TypeVar('T')

@dataclass
class SearchResult(Generic[T]):
    count: int
    items: list[T] = field(default_factory=list)

1c. Documentation

The documentation just shows a couple of example usages, but doesn't actually go into any details or best practices.
E.g. should you use the JSON endpoints? Do they even work?

Ideally most of the code should be auto-explanatory (e.g. due to being fully typed), and only the additional things could be documented.

1d. Optional: synthetic sugar

If you already have some helper dataclasses instead of dicts, it's possible to improve the quality of life with them.
E.g. currently you always have to create new objects using slugs. E.g. you search for some movie, you need to extract the slug from the result and create a new Movie object. It would be a lot simpler, if you just add a couple of helper methods like so:

@dataclass
class MovieRef:
    slug: str
    title: str
    year: int | None
    
    def fetch(self) -> Movie:
        return Movie.fetch_by_slug(self.slug)

1e. Optional: better search

A second idea - that I needed in my own project - was an intelligent search. Currently you can't actually filter by director, by year, ...
I wanted to find the correct movie for some given data, and if the search just returns a list the user needs to check which one is actually the correct one. Especially bad if there are multiple movies with the same name.

An experimental function like find_best_movie_match(title: str, approx_year: int | None = None, directors: str | None) would be cool.
I know that Letterboxd doesn't support such searches, but it's possible to just search for the title and filter the response.
(I made a somewhat working solution, but it's also not perfect.)

2. Developer Experience:

2a. Dependency Management: uv

There is already a modern pyproject.toml in use, but interestingly also still a requirement.txt for some installations.
Also for development there are libraries like pytest, ruff, ... used, but they are never installed (at least not with pyproject.toml).
=> use uv everywhere and add dev dependencies.

2b. Build tool: uv

If you already use uv, you can also use the build tool out of the box (instead of using hatchling).

2c. Testing with pytest?

Currently pytest is used in the pipeline to run the tests, but interestingly the tests are still written in a unittest-style way and there's the run.tests.sh together with the unittest.main() calls.
=> Just switch to pytest fully and modernize the tests.

2d. Recording Requests

Developing a library that makes requests is annoying, because you need to wait a long time every single time you run tests.
There's the VCR pattern. Basically you record the requests/responses during the first test run, and then just replay the requests during all consecutive test runs. I would even suggest to commit the responses (although you probably don't want to use them in the nightly runs - otherwise you wont realize if Letterboxd changes their HTML).

I've already tried this out, and by stripping some redundant HTML and compressing the files I reached between 10KB and 30KB per request (~380KB for all non-skipped tests together) and the tests run now in ~1 seconds.

2e. Remove dependency to fastfingertips

Having a dependency on a one-person helper library is quit ugly. Even (or especially) if it's your own library. That means that the library is no longer standing on it's own feet, and the devs have not possibilities to actually fix stuff and have no control over that code (e.g. you could inject some malicious code into the library, the dependency might contradict (e.g. currently your helper library requires rich, which should not be necessary for just the default letterboxd parser), etc... Maybe for the optional examples it's fine, but only as a library I would very much argue for kicking that library out.

2f. Publish library with "Trusted Publisher"

It looks like, the library is still published via PYPI API token. Which is not recommended any more.
Trusted Publisher is the way to go: https://docs.pypi.org/trusted-publishers/