Skip to content

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

# Conflicts:
#	pixivpy3/aapi.py
@Danipulok
Copy link
Contributor Author

@upbit I have updated the PR. Please review

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