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

feat(Onboarding): add edit method #1011

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

Conversation

Victorsitou
Copy link
Member

@Victorsitou Victorsitou commented Apr 19, 2023

Summary

discord/discord-api-docs#6101

blocked by #928

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running pdm lint
    • I have type-checked the code by running pdm pyright
  • 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, ...)

Victorsitou and others added 3 commits August 11, 2023 09:32
Co-authored-by: shiftinv <8530778+shiftinv@users.noreply.github.com>
Signed-off-by: Victor <67214928+Victorsitou@users.noreply.github.com>
@shiftinv
Copy link
Member

Any updates on this?

@Victorsitou Victorsitou requested a review from shiftinv March 22, 2024 16:06
disnake/onboarding.py Show resolved Hide resolved
disnake/onboarding.py Outdated Show resolved Hide resolved
disnake/onboarding.py Outdated Show resolved Hide resolved
disnake/onboarding.py Outdated Show resolved Hide resolved
disnake/onboarding.py Outdated Show resolved Hide resolved
Comment on lines 305 to 309
Accesing this during construction will raise an :exc:`ValueError`.
"""
if not self._guild or self.id == 0:
# TODO: better message and error?
raise ValueError("You cannot access this on construction.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Accesing this during construction will raise an :exc:`ValueError`.
"""
if not self._guild or self.id == 0:
# TODO: better message and error?
raise ValueError("You cannot access this on construction.")
Accessing this on manually constructed instances will raise a :exc:`ValueError`.
"""
if not self._guild:
raise ValueError("Roles cannot be resolved on manually constructed instances.")

something like this, maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's a discrepancy between user constructible and api-originating objects, then user constructible shouldn't have that parameter at all

Copy link
Member

Choose a reason for hiding this comment

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

@Enegg So what you're proposing is separating this into something like PartialOnboardingPromptOption (user constructible) and OnboardingPromptOption (api model)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mhm
Separate types provide a clear distinction of what a user can and cannot do

Copy link
Member

Choose a reason for hiding this comment

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

hmm, yeah. I agree with clear separation being better, however I'm not sure it's worth the effort here... it might be? With the way documentation works, this would mean a bunch of duplication both in the source code and in the built docs, for only a fairly small benefit, though.
The same topic also applies the polls implementation, might revisit this later after giving polls some more thought

disnake/onboarding.py Outdated Show resolved Hide resolved
@shiftinv shiftinv removed this from the disnake v2.10 milestone Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: needs review Issue/PR is awaiting reviews t: api support Support of Discord API features t: enhancement New feature
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants