Skip to content

Conversation

@goncalossilva
Copy link
Member

Because of some minor issues related to the integration between dataclasses-json and pyright, I ended up scouting their issue tracker, and was generally unimpressed — hundreds of issues, many without follow-up. I didn't find some areas of the code to be much better. The project doesn't seem like it's been actively maintainted for a while.

Searching for (lightweight) alternatives, I came across dataclass-wizard and I suggest we use it instead:

  • Faster than dataclasses-json: they claim 25X(!).
  • Smaller than dataclasses-json: it doesn't pull any additional dependencies`.
      + dataclass-wizard==0.35.0
      - dataclasses-json==0.6.7
      - marshmallow==3.26.1
      ~ todoist-api-python==2.1.7 (from file:///Users/goncalossilva/Code/Doist/todoist-api-python)
      - typing-inspect==0.9.0
    
  • Opinionated and modern (e.g., it includes full support for aware date times)
  • Good code and well-groomed backlog.

@goncalossilva goncalossilva requested a review from lefcha March 31, 2025 15:55
@goncalossilva goncalossilva requested a review from a team as a code owner March 31, 2025 15:55
@goncalossilva goncalossilva requested a review from Copilot March 31, 2025 15:58
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates the project from using dataclasses-json to dataclass-wizard for JSON serialization, addressing integration issues with pyright while potentially improving performance and dependency footprint.

  • Replace DataClassJsonMixin with JSONPyWizard in all model classes
  • Update dependency declarations in pyproject.toml and adjust documentation accordingly
  • Modify Due and Task conversion logic to align with dataclass-wizard usage

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

File Description
todoist_api_python/models.py Migration of model classes to use dataclass-wizard along with minor logic changes
pyproject.toml Update dependency from dataclasses-json to dataclass-wizard
README.md Update supported Python versions statement
CHANGELOG.md Change changelog entry to reflect migration to dataclass-wizard
Comments suppressed due to low confidence (2)

todoist_api_python/models.py:57

  • The post_init method in the Due class now sets 'self.datetime' to 'self.date' when both date and timezone are present, which might not fully replicate the previous behavior. Please confirm that this simplified assignment meets the intended logic.
if not self.datetime and (self.date and self.timezone):

todoist_api_python/models.py:93

  • The removal of the conversion for the 'due' field in Task.from_quick_add_response could result in unprocessed due data. Verify that due data is correctly initialized using the new dataclass-wizard setup.
def from_quick_add_response(cls, obj: dict[str, Any]) -> Task:

@goncalossilva
Copy link
Member Author

As in before PRs, I'm merging so that I can keep iterating fast, but I would appreciate a review after the fact. Happy to make any improvement or accommodate any change we see fit.

@goncalossilva goncalossilva merged commit bb6597c into main Mar 31, 2025
1 check passed
@goncalossilva goncalossilva deleted the dataclass-wizard branch March 31, 2025 15:59
@goncalossilva
Copy link
Member Author

  • The post_init method in the Due class now sets 'self.datetime' to 'self.date' when both date and timezone are present, which might not fully replicate the previous behavior. Please confirm that this simplified assignment meets the intended logic.

if not self.datetime and (self.date and self.timezone):

This is actually a good point by Copilot. Our updated code DOES replicate the old logic, but the old logic is pretty bizarre. 😅

Copy link
Collaborator

@lefcha lefcha left a comment

Choose a reason for hiding this comment

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

Seems reasonable, but I wonder if it's worth all the hassle since we plan on moving to Pydantic anyway...

@goncalossilva
Copy link
Member Author

I'm not sure if we'll move to Pydantic. At least after reading @proxi's feedback, I don't it makes that much sense. It's quite a big dependency for almost no direct benefit to the consumer of the SDK, and we can get most of the advantages we'd want using a lighterweight alternative. The way I see it, this would stay for now. 🤔

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