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

Add external engine analysis post endpoints #81

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

Conversation

FriedrichtenHagen
Copy link

Checklist when adding a new endpoint Building on @Anupyas PR I moved the analysis endpoints into the external_engine client. I handled the different base URL with a new parameter in the Requestor methods post and request which accepts the different base url.
  • Added new endpoint to the README.md
  • Ensure that my endpoint name does not repeat the name of the client. Wrong: client.users.get_user(), Correct: client.users.get()
  • Typed the returned JSON using TypedDicts in berserk/types/, example
  • Tested GET endpoints not requiring authentification. Documentation, example
  • Added the endpoint and your name to CHANGELOG.md in the To be released section (to be created if necessary)

@fitztrev
Copy link
Member

Could custom_base_url be overridden to test on a local dev site?

@FriedrichtenHagen
Copy link
Author

Hi @fitztrev, I am not sure if I understand what you mean.
I believe the custom_base_url parameter is only necessary for the three new endpoints (analysis, aquire_request and answer_request). I am not aware of other endpoints that would need a base url that differs from the standard (https://lichess.org/). But I am new to this project, so please let me know if there is something I have overlooked.

@fitztrev
Copy link
Member

I mean when someone goes to use berserk in a script, they are currently able to specify a custom base_url instead of pointing to production lichess.org.

See example here that points to http://nginx https://github.com/lichess-org/lila-docker/blob/main/scripts/berserk-example.py

@FriedrichtenHagen
Copy link
Author

@fitztrev Yes, should be no problem. That custom base url would need to be passed as an argument (custom_base_url) when calling one of the new endpoints methods (analysis, aquire_request and answer_request).
The idea is to use this additional base url in addition to the main base url which is used by all other endpoints.

@benediktwerner
Copy link
Member

I think it makes more sense to do it the same as with the tablebase or opening explorer where the custom URL can be set on the berserk.Client:

https://github.com/lichess-org/berserk/blob/master/berserk/clients/__init__.py#L91

https://github.com/lichess-org/berserk/blob/master/berserk/clients/tablebase.py

Unfortunately, some external engine requests have to go against the default Licehss URL but I think it also still makes sense to use a similar pattern with a separate client that has the correct base URL set instead of passing it on every call. The ExternalEngine client could create a separate sub-client in its constructor with the external engine URL as its base URL. Or ExternalEngine could simply not subclass BaseClient and instead have two clients as attributes. I think either option is fine.

Then we don't need custom URL params on any of the methods. And the default external engine URL can also be extracted to a constant at the top of the external engine file.

@FriedrichtenHagen
Copy link
Author

@benediktwerner Thank you for the explanation, that makes sense and is indeed cleaner.
I have added a subclient as recommended and removed the use of custom_base_url. The external engine url is extracted to the top of the file.

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.

3 participants