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

feat(models): add models & upgrade minimum python version to 3.8 #377

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Danipulok
Copy link
Contributor

What this PR does?

  • Change minimal python version from 3.7 to 3.8 to support pydantic.
  • Add pydantic and validation for most common methods.

What this PR does in details?

  • As a base, I used models from 尝试为部分aapi.py中的方法添加TypedDict类型注解 #355;
  • Remove all mention of python 3.7;
  • Add type for Response: Anyrequests.Response;
  • Create file models.py dedicated for all the models;
  • Add type-hints to pixivpy3/api.py to pass pre-commit mypy checks.
  • Add helper methods load_model and load_result to AppPixivAPI;
  • poetry changed format in 2.x, so the poery.lock seems big. But only pydantic is added;

Breaking changes:

  • Now the objects are actual objects and support getattr only in format obj.field, not obj["field"]. Maybe I can add getattr for backward compatibility, or you can change library's major version? I also should update the docs (not done yet)
  • novel_text now returns object in snake_case, not camelCase. Allow both ways for backward compatibility, maybe?

What more should be done\discussed:

  • Change all example from obj["attr"] to obj.attr;
  • Maybe add more models? I covered most basic methods and want to know your opinion about this PR in general, before covering all the models (or do it later / in another PR / keep it like this). I would really like it to accepted like this, so at least part of the job is done.

Reasoning:

I really liked your library, and thanks for making and maintaining this project!

I want to make the library easier to use, more stable and user-friendly, that's why I've decided to add validation and actual objects.

PS:

If there is anything you wish for me to change / add, please tell me, and I will try to do it, because I really would like for this PR to get accepted.

@upbit
Copy link
Owner

upbit commented Jan 13, 2025

I'm glad you like this project and welcome your contributions!

Providing the definition of models.py is great, as it helps to clearly understand the returned fields (in fact, this is a significant amount of work, requiring the organization of the return parameters for various APIs, as well as synchronizing updates; I previously took a shortcut by using aapi.parse_qs to adapt to Pixiv's new parameters).


One point of concern is the introduction of pydantic = "^2.0.0". We import v2 could create compatibility issues for repositories still using pydantic v1, since it's problematic to include both versions in the same project.

Early versions of langchain used the built-in pydantic_v1 library to avoid this problem:

from pydantic.v1 import BaseModel # <-- Note v1 namespace

and langchain-core>=0.2.23 now has a compatible solution, just import from pydantic import BaseModel. So I may need to confirm how this integration was achieved.


Additionally, the usage of obj["attr"] and obj.attr in the example should also be compatible with the current version, as it achieves a similar effect through the dict __getattr__ method.

@Danipulok
Copy link
Contributor Author

Danipulok commented Jan 13, 2025

@upbit thank you very much for taking the PR into consideration!

Yes, I can make Pydantic compatible with 1.x versions too. And make the fields accessible as in dict too.

Is there anything else I should change?

I'm also worried about having the response model different in novel_text. Is it okay now it has snake_case fields? Just want to make sure it's okay with you. I guess I can make fields in that specific model accessible via camelCase if you wish.

Also I have some small file that calls all the methods so it's easier to check them and response models. Should I add it to the repo?

@upbit
Copy link
Owner

upbit commented Jan 13, 2025

I'm happy to discuss solutions with you. Do you know how the new version of LangChain(langchain-core>=0.2.23) solves the Pydantic adaptation issue for v1? Currently, I haven't had time to look at the code of the new version of LangChain.

Sure. If you already have unit tests that can verify the correctness of the fields, you can submit them together (and configure the pipeline check).

Regarding the naming convention of the fields, my view is that we can currently maintain consistency with the snake_case style returned by Pixiv, and later supplement it with camelCase to match the code style guide.

@Danipulok
Copy link
Contributor Author

I'm familiar with how compatibility is done. Usually, authors just create files like compat.py where they handle imports from both version of the Pydantic. Here are some examples:

I will do something similar in the models.py to not spread unneeded imports and checks etc.

@Danipulok
Copy link
Contributor Author

Danipulok commented Jan 13, 2025

Sure. If you already have unit tests that can verify the correctness of the fields, you can submit them together (and configure the pipeline check).

Unfortunately, it's not unit tests, just a file that calls all methods
I just thought it would be useful in case of new models from Pixiv to check everything quickly
image

Regarding the naming convention of the fields, my view is that we can currently maintain consistency with the snake_case style returned by Pixiv, and later supplement it with camelCase to match the code style guide.
As I understand, the fields are always snake_case. But in novel_text method, where the model is parsed from HTML, the fields are camelCase style. It's the only model in the library where fields are like this, as I have understood. So my question is not about the models in general, but only NovelText.

Because old usage:

novel = api.novel_text(...)
novel.someField

New usage:

novel = api.novel_text(...)
novel.some_field

I can add handling both cases for this specific model

@upbit
Copy link
Owner

upbit commented Jan 13, 2025

a file that calls all methods it is enough, which is simple and effective.

I haven't noticed the case of novel_text. If it is novel.someField returned by Pixiv itself, you can ignore it for now.

Later we will see if there is a unified solution to convert the getter/setter of snake_case and camelCase (of course, it should not affect the performance too much)

@Danipulok
Copy link
Contributor Author

@upbit hm, I just don't quite understand what's the point in having all fields being accessible via camelCase too if it's Python and all the fields should be snake_case. Is there any specific reason you wish for that?

I can add this too and for all models (not just NovelText). I just don't understand the reason and think it will only make the code and accessibility worse, since there are two ways to do the same thing

@Danipulok
Copy link
Contributor Author

@upbit, I have updated all the code. Please check. Also, please check #379 at first

@Danipulok
Copy link
Contributor Author

Danipulok commented Jan 14, 2025

@upbit I have updated this and all my other commits. Please check when possible

@upbit
Copy link
Owner

upbit commented Jan 15, 2025

This commit involves significant changes. I'll keep it here first to test the API's forward compatibility.

@Danipulok
Copy link
Contributor Author

Danipulok commented Jan 15, 2025

Okay, got it. Currently, the single breaking change is having fields in novel_text snake_case and not camelCase. All other is the same. Will be waiting for updates from you

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

Successfully merging this pull request may close these issues.

2 participants