-
Notifications
You must be signed in to change notification settings - Fork 2
python: update README #415
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
Conversation
python/README.md
Outdated
|
|
||
| # connect to a non-default port on the local network | ||
| aica = AICA(port=55000) | ||
| aica = AICA(url='http://localhost:55005/api') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well if anything, this constructor should also do the api_key= which has become mandatory in v4 of the api client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should add it in every AICA() call from there to the top? If yes, shouldn't we just move the Authentication section on top, even maybe before Basic Usage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that might be a good thing to do now that it has become required. We could then mention that this was not always the case instead of the other way around
python/README.md
Outdated
|
|
||
| # connect to a non-default port on the local network | ||
| aica = AICA(url='http://localhost:55005/api') | ||
| aica = AICA(url='http://localhost:55005/api', api_key=AICA_API_KEY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you switch the arguments around here please, even though not striclty required
domire8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me but happy for at least one other review 👍 thanks
Description
Small fixes in the README that I noticed. More of a draft, if you notice there is something else to fix, we can probably squeeze it here.
Review guidelines
Estimated Time of Review: x5minutes
Checklist before merging: