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

Improvements #143

Open
lucas-montes opened this issue Dec 18, 2022 · 1 comment
Open

Improvements #143

lucas-montes opened this issue Dec 18, 2022 · 1 comment

Comments

@lucas-montes
Copy link

First of all, great work. As for the facebook one, this library is really cool.

Duplicated code:
I haven't dig dipper, but it seems that there is a lot of duplicated code.

  • The resources module has many classes with the same method list, update, etc... maybe that could be moved into a parent class where all the verifications are done at this level.
  • The Client and API classes. They seem very similar. API seems to have methods that for the Client class should be called through a resource. It might be worth to separate the auth part (requesting tokens, refreshing them and so on) into an independent class, then the API class could be removed and use only the Client one as they would do the same thing.

Method's parameters

  • Instead of allowing None values and the checking if the value is None setting a default value, I think that it would be better to set the default value on the parameter itself.

Reduce comments

  • Many methods have comments that are redundant, the method names and parameters are usually clear enough. Nevertheless it might be helpful having a link to the youtube docs.
@MerleLiuKun
Copy link
Member

The 'Client' class is a new implementation, while the 'Api' class is an older version. I believe the 'Api' class will be removed in the future.

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

No branches or pull requests

2 participants