Skip to content

Conversation

@advaith1
Copy link
Contributor

Document server template routes that bots could not access before (#1712 (comment))

@tpcstld tpcstld self-assigned this Oct 11, 2020
@Nihlus
Copy link
Contributor

Nihlus commented Oct 11, 2020

Regarding the serialized_source_guild property, it's relatively unclear without looking at the returned JSON which properties may or may not be present (and that continues down into roles and channels of that object as well). Perhaps it's worth documenting those three separately from the main guild object? Another motivation for that would be that the main guild object contains a number of fields that would be completely irrelevant for a template, such as the owner ID and the permissions of the current user.

To be fair, this is an issue with most objects marked as partial, and probably warrants a general once-over, but I think it's worth considering in this PR as well.

advaith1 and others added 2 commits October 13, 2020 18:07
Co-authored-by: Jerry Jiang <jerry@tpcstld.me>
@tpcstld
Copy link
Contributor

tpcstld commented Oct 14, 2020

Nihlus details a good concern, and unfortunately I have no good solution here.

In general for this set of APIs, the serialized_source_guild property is very messy. Internally, it is inherited from the base Guild object, and Discord clients parse it like a Guild (with the exception of icon/icon_hash) so I don't think that documenting it completely separately is a good idea. If the base Guild ever had a new non-optional field, serialized_source_guild would likely come with it as well.

A half-and-half approach I was thinking about is adding a new data model that kind of explains "A serialized source guild is a Guild with these additional fields", but I don't think there's precedent for this kind of writing in these docs so I'm not too willing to introduce it here right now.

I agree with Nihlus, as Discord's features and endpoints become more and more complex, I definitely believe that we need to refactor the docs to make these inheritance chains more clear and understandable. However right now I'm going to kick this ball a bit further down the road, because I'd like some more data points on this issue (i.e. I want to see the next problem), and because I don't feel comfortable personally reviewing such foundational changes right now.

@ajpalkovic
Copy link
Contributor

The format of serialized_source_guild is exactly the same as what the create guild endpoint accepts: https://discord.com/developers/docs/resources/guild#create-guild

It's a template, it's purpose is ... creating guilds, so the data format is just exactly what we need in order to re-create that guild. So the same rules/restrictions/conventions that the create-guild endpoint has apply here too (for example the first role is always for @-everyone, etc)

icon/icon_hash are the one exception, but those are ignored for guild templates, we never copy over the icon anymore

@Nihlus
Copy link
Contributor

Nihlus commented Oct 14, 2020

Nihlus details a good concern, and unfortunately I have no good solution here.

In general for this set of APIs, the serialized_source_guild property is very messy. Internally, it is inherited from the base Guild object, and Discord clients parse it like a Guild (with the exception of icon/icon_hash) so I don't think that documenting it completely separately is a good idea. If the base Guild ever had a new non-optional field, serialized_source_guild would likely come with it as well.

A half-and-half approach I was thinking about is adding a new data model that kind of explains "A serialized source guild is a Guild with these additional fields", but I don't think there's precedent for this kind of writing in these docs so I'm not too willing to introduce it here right now.

I agree with Nihlus, as Discord's features and endpoints become more and more complex, I definitely believe that we need to refactor the docs to make these inheritance chains more clear and understandable. However right now I'm going to kick this ball a bit further down the road, because I'd like some more data points on this issue (i.e. I want to see the next problem), and because I don't feel comfortable personally reviewing such foundational changes right now.

@tpcstld

I have some thoughts on the matter, but it might be better to move that conversation to a new issue since it's probably going to be a rather complex and/or extensive discussion about what to do. In short, though, I'd be mostly happy with one of two things happening.

  1. More granular types, expressing inheritance between them
  2. More detailed and encompassing optionality annotations of the existing types. This, for example, would mean that id in the Guild object would become an optional property to match this definition

Would you like me to open an issue and elaborate a bit more?

@ajpalkovic

That makes sense, but also weighs in favor of breaking this out into a new type (Guild Template or Template Guild, perhaps?), since it's not exactly meant to describe an existing guild, which the base guild type ostensibly is.

@tpcstld
Copy link
Contributor

tpcstld commented Oct 15, 2020

@Nihlus

Would you like me to open an issue and elaborate a bit more?

I think this is a good idea. I expect this concern to come up again and it definitely looks like a much bigger scope than this PR.

But to set some expectations I'm going to avoid personally making any decisions around this area. I'm honestly not too familiar with the API, and so I'm definitely going to defer to the longstanding API doc maintainers (i.e. night and mason). But they are very busy right now so nothing will probably happen soon :P

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants