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

refactor(client): add _url & _ajax_url helper methods #384

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

Conversation

Danipulok
Copy link
Contributor

What this PR does?

  • Add _url and _ajax_url helper methods to the class to omit boilerplate formatting;

TODO:

After this PR is merged (I hope), I would like to make all constants actual constants. It will allow us to reuse code and have it in one place even more

@upbit
Copy link
Owner

upbit commented Jan 14, 2025

There are significant differences in the interfaces and fields between www.pixiv.net and Pixiv App.
Pixivpy is designed based on the App's API, and currently, there are no plans for migrating to Web API, or using it in a mixed way.

@upbit upbit added the wontfix label Jan 14, 2025
@Danipulok
Copy link
Contributor Author

Got it, thanks for claryfying. There's already a method that uses Web API (showcase). So the idea is that if it's already used (I did not add it) is to make it kinda more generic

What about the methods like comments from my other PR that returns 404 now? And that method with showcase, which uses Web API and already in the class anyway...

@Danipulok
Copy link
Contributor Author

@upbit please tell me what you think about adding just _url then?
IMHO it's useful to format the url in a single place
Or should I just close this PR?

# Conflicts:
#	pixivpy3/aapi.py
@upbit
Copy link
Owner

upbit commented Jan 14, 2025

@upbit please tell me what you think about adding just _url then? IMHO it's useful to format the url in a single place Or should I just close this PR?

Both def _url(self, endpoint: str) -> str: and const hostname are good, avoiding the problem of modifying string magic numbers everywhere.

@upbit upbit removed the wontfix label Jan 15, 2025
@Danipulok
Copy link
Contributor Author

Danipulok commented Jan 15, 2025

@upbit I have removed the _ajax_url method. I was planning to do consts in another commit sooner. In a separate commits for all consts

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.

2 participants