-
Notifications
You must be signed in to change notification settings - Fork 150
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
base: master
Are you sure you want to change the base?
Conversation
I'm glad you like this project and welcome your contributions! Providing the definition of One point of concern is the introduction of Early versions of langchain used the built-in from pydantic.v1 import BaseModel # <-- Note v1 namespace and langchain-core>=0.2.23 now has a compatible solution, just import Additionally, the usage of |
@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 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? |
…kward compatibility
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 |
I'm familiar with how compatibility is done. Usually, authors just create files like I will do something similar in the |
I haven't noticed the case of Later we will see if there is a unified solution to convert the |
@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 |
# Conflicts: # pixivpy3/aapi.py # poetry.lock
@upbit I have updated this and all my other commits. Please check when possible |
This commit involves significant changes. I'll keep it here first to test the API's forward compatibility. |
Okay, got it. Currently, the single breaking change is having fields in |
What this PR does?
3.7
to3.8
to supportpydantic
.pydantic
and validation for most common methods.What this PR does in details?
3.7
;Response
:Any
→requests.Response
;models.py
dedicated for all the models;pixivpy3/api.py
to passpre-commit
mypy
checks.load_model
andload_result
toAppPixivAPI
;poetry
changed format in2.x
, so thepoery.lock
seems big. But onlypydantic
is added;Breaking changes:
getattr
only in formatobj.field
, notobj["field"]
. Maybe I can addgetattr
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 insnake_case
, notcamelCase
. Allow both ways for backward compatibility, maybe?What more should be done\discussed:
obj["attr"]
toobj.attr
;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.