-
Notifications
You must be signed in to change notification settings - Fork 6.4k
[DRAFT] [FEEDBACK REQUESTED] Add calendar_id
for querying calendars
#19978
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
base: main
Are you sure you want to change the base?
[DRAFT] [FEEDBACK REQUESTED] Add calendar_id
for querying calendars
#19978
Conversation
@logan-markewich Thank you for your help in a previous PR for the google tools integration. Could you please advise what the minimum effort for maintainability best practices is required here? (full ask in PR description) |
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.
Looks good, just some minor comments :) 👇
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.
I see that we previously used "primary" as an id: can we default to that instead of returning the error?
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.
Same comment as above :)
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.
We should move imports at the top of the module
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.
Imports at the top of the module
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.
Naming nit: I would probably call it list_all_calendars
or get_all_calendars
Hi! I am looking for conceptual review before I make a PR with an agreed upon approach.
I have added calendar ID option to the list calendars function. This is technically a breaking change. However, in my testing the calendar id
"primary"
does not work. According to my tests and the documentation, it must be an email:https://developers.google.com/workspace/calendar/api/concepts/events-calendars#calendars
I would like to take the easy path and use the current approach.
A technically safer approach would be to make a new class that doesn't "break" compatibility with existing consumers.
Or to make a different function.
Thoughts?