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

docs(TextChannel): warning about setRateLimitPerUser NewsChannel #5403

Merged
merged 6 commits into from
May 29, 2021

Conversation

ImRodry
Copy link
Contributor

@ImRodry ImRodry commented Mar 14, 2021

Please describe the changes this PR makes and why it should be merged:
This adds a warning to the docs denoting that setRateLimitPerUser is not available on NewsChannels

Status and versioning classification:

  • I know how to update typings and have done so, or typings don't need updating
  • This PR changes the library's interface (methods or parameters added)

This fixes an issue with the docs where the rateLimitPerUser property and the setRateLimitPerUser method would show up for NewsChannel despite them not existing in it
@vladfrangu
Copy link
Member

Copy-pasting the entire text channel just to remove one property doesn't seem like the ideal solution for this at all.. If anything, the docs should explain that NewsChannels lack the property through some way

@ImRodry
Copy link
Contributor Author

ImRodry commented Mar 14, 2021

Copy-pasting the entire text channel just to remove one property doesn't seem like the ideal solution for this at all.. If anything, the docs should explain that NewsChannels lack the property through some way

From what I understood the docs take everything from the code itself so, if NewsChannel extends TextChannel, every property will have the exact same description. I know copy-pasting is not good practice but I think it's necessary in this case due to the already existent inconsistencies in the NewsChannel typings.

Copy link

@ProFireDev ProFireDev left a comment

Choose a reason for hiding this comment

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

"Copy-pasting the entire text channel just to remove one property doesn't seem like the ideal solution for this at all.. If anything, the docs should explain that NewsChannels lack the property through some way"

this is not the best way of doing it, as such it shouldn't be merged.. i would say that its best if this PR were to get closed, and then maybe you or someone else can comeback with a better solution.

@ImRodry
Copy link
Contributor Author

ImRodry commented Mar 22, 2021

"Copy-pasting the entire text channel just to remove one property doesn't seem like the ideal solution for this at all.. If anything, the docs should explain that NewsChannels lack the property through some way"

this is not the best way of doing it, as such it shouldn't be merged.. i would say that its best if this PR were to get closed, and then maybe you or someone else can comeback with a better solution.

Do you know what solution would be better? I'd be glad to do that if there was one, I just couldn't find any other way to completely remove a property from a class in the docs without doing this :/

@ProFireDev
Copy link

ProFireDev commented Mar 22, 2021

"Copy-pasting the entire text channel just to remove one property doesn't seem like the ideal solution for this at all.. If anything, the docs should explain that NewsChannels lack the property through some way"
this is not the best way of doing it, as such it shouldn't be merged.. i would say that its best if this PR were to get closed, and then maybe you or someone else can comeback with a better solution.

Do you know what solution would be better? I'd be glad to do that if there was one, I just couldn't find any other way to completely remove a property from a class in the docs without doing this :/

ill have to read though the code more and try and figure it out but the following was on to something.

Copy-pasting the entire text channel just to remove one property doesn't seem like the ideal solution for this at all.. If anything, the docs should explain that NewsChannels lack the property through some way

I think updating the docs would go a long way to make this more clear, but I would encourage you to ask in the discord too. they might be able to help you out better or come up with a solution.

discord invite

@ImRodry
Copy link
Contributor Author

ImRodry commented Mar 22, 2021

I initially asked a support agent there and this is the solution he gave me so I'm not really sure who else to ask :/
I think ideally the docs would need an update to allow for properties to be removed instead of blindly taking it from the code.

@ProFireDev
Copy link

I initially asked a support agent there and this is the solution he gave me so I'm not really sure who else to ask :/
I think ideally the docs would need an update to allow for properties to be removed instead of blindly taking it from the code.

@almostSouji do you think this would be possible?

You seem to be the one that contributed to the guide most recently, so I'm assuming you would have a say in something like this.

As for @ImRodry if the docs won't get updated, I would suggest closing this PR and opening a new issue instead.

@ImRodry
Copy link
Contributor Author

ImRodry commented Mar 22, 2021

Sure thing, I'll wait for that guy's reply and if the docs can't be updated I'll do that, thank you for the feedback!

Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

Since it seems you're waiting for the team's consensus, after internal discussions, we decided that Vlad's approach at #5403 (comment) is desirable.

@ImRodry
Copy link
Contributor Author

ImRodry commented May 10, 2021

@kyranet how should we do that though?

@kyranet
Copy link
Member

kyranet commented May 10, 2021

@kyranet how should we do that though?

The issue is about the docs not explaining the lack of the property, so instead of duplicating a lot of code for documentation purposes, you can add some documentation to explain why the property is missing, and that'll solve the issue you described in this PR.

@ImRodry
Copy link
Contributor Author

ImRodry commented May 10, 2021

Well ideally we would remove the property entirely but a warning can also work. Would it be possible to update the docs to somehow detect that a property was removed from a class?

@ImRodry ImRodry changed the title Extend NewsChannel from GuildChannel Add warning to setRateLimitPerUser about NewsChannel May 10, 2021
@ImRodry
Copy link
Contributor Author

ImRodry commented May 10, 2021

@kyranet tried a different and simpler approach, let me know what you think!

@ImRodry
Copy link
Contributor Author

ImRodry commented May 10, 2021

Along with that, I also noticed that this warning is currently ineffective as it must be placed on GuildChannel instead but I won't add it to this PR as it's not related (unless you want me to)

Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

Problem with the docs as they're written right now is that we get an out-of-scope warning: the behaviour affects NewsChannel exclusively, but is present in TextChannel.

src/structures/TextChannel.js Show resolved Hide resolved
@ImRodry
Copy link
Contributor Author

ImRodry commented May 10, 2021

Problem with the docs as they're written right now is that we get an out-of-scope warning: the behaviour affects NewsChannel exclusively, but is present in TextChannel.

Yeah but if we added that to a JSDoc on NewsChannel it would be overwritten by the one on TextChannel. You can see this on the line I linked above

@ImRodry ImRodry requested a review from kyranet May 29, 2021 14:20
@iCrawl iCrawl changed the title Add warning to setRateLimitPerUser about NewsChannel docs(TextChannel): warning about setRateLimitPerUser NewsChannel May 29, 2021
@iCrawl iCrawl merged commit 47bbdf4 into discordjs:master May 29, 2021
@ImRodry ImRodry deleted the master branch June 20, 2021 11:32
@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
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.

None yet

8 participants