Skip to content

Conversation

@DA-344
Copy link
Contributor

@DA-344 DA-344 commented Mar 23, 2025

Summary

Adds support for receiving, setting, and updating a Scheduled Event's recurrence rule.

Documentation: resources/guild-scheduled-event

Needs testing.

Information

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed).
  • This PR is not a code change (e.g. documentation, README, typehinting,
    examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.
  • I have updated the changelog to include these changes.

DA-344 and others added 3 commits April 1, 2025 09:53
Co-authored-by: plun1331 <plun1331@gmail.com>
Signed-off-by: DA344 <108473820+DA-344@users.noreply.github.com>
Comment on lines +321 to +322
If this recurrence rule was obtained from the API you will need to
:meth:`.copy` it in order to edit it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more question... why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this

Copy link
Member

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.

@Lulalaby
Copy link
Member

Lulalaby commented Aug 2, 2025

Merge conflicts
(complicated discord feature, requires intense testing)

@Paillat-dev Paillat-dev added API reflection Discord API isn't correctly reflected hold: testing This pull request requires further testing labels Aug 6, 2025
@Lulalaby Lulalaby requested a review from a team as a code owner August 30, 2025 20:33
@Lulalaby Lulalaby force-pushed the master branch 2 times, most recently from b55c125 to 82659b2 Compare August 30, 2025 21:10
@Lulalaby Lulalaby removed the on hold label Aug 30, 2025
@Lulalaby Lulalaby requested a review from a team as a code owner September 1, 2025 12:31
@Paillat-dev Paillat-dev self-assigned this Sep 1, 2025
@Paillat-dev Paillat-dev self-requested a review September 1, 2025 15:41
Signed-off-by: Lala Sabathil <lala@pycord.dev>
@Lulalaby
Copy link
Member

Lulalaby commented Sep 1, 2025

@DA-344 went ahead and fixed the merge conflicts for you. please double check

Comment on lines +321 to +322
If this recurrence rule was obtained from the API you will need to
:meth:`.copy` it in order to edit it.
Copy link
Member

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`]
Copy link
Member

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 ?

Lumabots and others added 3 commits October 20, 2025 22:01
Signed-off-by: Lumouille <144063653+Lumabots@users.noreply.github.com>
Signed-off-by: Lala Sabathil <lala@pycord.dev>
Copy link
Member

@Paillat-dev Paillat-dev left a 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]:
Copy link
Member

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

Comment on lines +321 to +322
If this recurrence rule was obtained from the API you will need to
:meth:`.copy` it in order to edit it.
Copy link
Member

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 [],
Copy link
Contributor

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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

Copy link
Member

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],
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

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__(
Copy link
Member

@Paillat-dev Paillat-dev Dec 12, 2025

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,
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API reflection Discord API isn't correctly reflected hold: testing This pull request requires further testing priority: medium Medium Priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants