-
-
Notifications
You must be signed in to change notification settings - Fork 480
feat: Add support for Guild Scheduled Event Recurrence #2749
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: master
Are you sure you want to change the base?
Conversation
Co-authored-by: plun1331 <plun1331@gmail.com> Signed-off-by: DA344 <108473820+DA-344@users.noreply.github.com>
… into feat/recurrence-rule
| If this recurrence rule was obtained from the API you will need to | ||
| :meth:`.copy` it in order to edit it. |
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.
one more question... why?
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.
this
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.
This class seems to serve more as a dataclass-ish than an API object. Which is fine, but having an edit method here, and then disallowing editing attributes seems weird. This class should realistically not even hold self._state (I believe). It would probably make everything easier.
|
Merge conflicts |
b55c125 to
82659b2
Compare
Signed-off-by: Lala Sabathil <lala@pycord.dev>
|
@DA-344 went ahead and fixed the merge conflicts for you. please double check |
| If this recurrence rule was obtained from the API you will need to | ||
| :meth:`.copy` it in order to edit it. |
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.
this
| image: Optional[:class:`bytes`] | ||
| The cover image of the scheduled event | ||
| The cover image of the scheduled event. | ||
| recurrence_rule: Optional[:class:`ScheduledEventRecurrenceRule`] |
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.
If this is provided, is start_date ignored ?
Signed-off-by: Lumouille <144063653+Lumabots@users.noreply.github.com>
Signed-off-by: Lala Sabathil <lala@pycord.dev>
Paillat-dev
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.
Also see merge conflicts
| return f"<ScheduledEventRecurrenceRule start_date={self.start_date} frequency={self.frequency} interval={self.interval}>" | ||
|
|
||
| @property | ||
| def weekdays(self) -> list[ScheduledEventWeekday]: |
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've been thinking about this again. If you can, I think we should just make these normal attributes. Realistically, it's more likely to confuse someone than prevent misuse. See my comment on edit below as well
| If this recurrence rule was obtained from the API you will need to | ||
| :meth:`.copy` it in order to edit it. |
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.
This class seems to serve more as a dataclass-ish than an API object. Which is fine, but having an edit method here, and then disallowing editing attributes seems weird. This class should realistically not even hold self._state (I believe). It would probably make everything easier.
Co-authored-by: Paillat <paillat@pycord.dev> Signed-off-by: DA344 <108473820+DA-344@users.noreply.github.com>
| self.exceptions: list[Object] = list( | ||
| map( | ||
| Object, | ||
| data.get("guild_scheduled_events_exceptions") or [], |
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 cant find any docs on guild_scheduled_events_exceptions, could you link one to me ?
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.
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 so its not documented by discord it shouldnt be here imo
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.
Yep this
| self, | ||
| start_date: datetime.datetime, | ||
| frequency: ScheduledEventRecurrenceFrequency, | ||
| interval: Literal[1, 2], |
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.
This could be made an enum
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.
There are no names for each interval, so an enum does not work here imo
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.
But can interval just be 1 or 2 ? I assumed that and that's why I commented that.
It should be our goal to provide enums to end users so that they don't have to add magic values. We can still add a union with that Literal, and use an IntEnum if needed, and type the attribute as a Literal only.
| "_state", | ||
| ) | ||
|
|
||
| def __init__( |
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.
Also (don't wanna sound painful 😭) but could be neat if this had some overloads to enforce presence / absence of params
| *, | ||
| weekdays: list[WeekDay | ScheduledEventWeekday] = MISSING, | ||
| n_weekdays: list[NWeekDay] = MISSING, | ||
| month_days: list[datetime.date] = MISSING, |
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.
How does this work with two different days on two different months ? Realistically that would end up to 4x a year instead of 2x a year when casted into discord's api's format
Summary
Adds support for receiving, setting, and updating a Scheduled Event's recurrence rule.
Documentation: resources/guild-scheduled-event
Needs testing.
Information
examples, ...).
Checklist
type: ignorecomments were used, a comment is also left explaining why.