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(ApplicationCommand): fix and improve localization docs #7804

Merged
merged 1 commit into from Apr 21, 2022

Conversation

ImRodry
Copy link
Contributor

@ImRodry ImRodry commented Apr 16, 2022

Please describe the changes this PR makes and why it should be merged:

This PR:

  • adds a Locale typedef with all the possible locales to make it easier to document them in various places
  • fixes the documentation of the *Localizations and *Localized properties

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

@xHyroM
Copy link
Contributor

xHyroM commented Apr 16, 2022

If this PR gets an approve, I'll do a backport for v13.

@suneettipirneni
Copy link
Member

I tested with a bot token and the X-Discord-Locale header and they are not sent

I just tested this and it certainly sends back name_localized and description_localized when the header is set.

changes the default of withLocalizations in ApplicationCommandManager#fetch to true in order to always ensure the user running this method gets all the data, seeing as providing it as false by default has no impact other than receiving the properties as null

This was brought up internally and the consensus was that it shouldn't return localizations by default, because localization JSON can result in much larger json payloads. If you need to do something like diff commands you can specify the option as true in those situations.

@ImRodry
Copy link
Contributor Author

ImRodry commented Apr 16, 2022

I just tested this and it certainly sends back name_localized and description_localized when the header is set.

Show me how you did it then because I tested this many times and never received those fields

This was brought up internally and the consensus was that it shouldn't return localizations by default, because localization JSON can result in much larger json payloads. If you need to do something like diff commands you can specify the option as true in those situations.

That's inconsistent with other methods but if a maintainer wants that removed I'll remove it. I'll wait for now

@suneettipirneni
Copy link
Member

Show me how you did it then because I tested this many times and never received those fields

I just did a get request to fetch all commands with my bot token in the header and the locale header set to the locale.

@ImRodry
Copy link
Contributor Author

ImRodry commented Apr 16, 2022

Can't reproduce so unless someone else is able to and details how I'll keep these changes here. For reference I'm requesting all application commands without the with_localizations query parameter, a bot token and the X-Discord-Locale header set to "pt-BR"

@kyranet
Copy link
Member

kyranet commented Apr 17, 2022

That's inconsistent with other methods but if a maintainer wants that removed I'll remove it. I'll wait for now

Hi, maintainer here. The behaviour and defaults of the method were chosen internally, and as such, we took part of it.

IMO the code changes in this PR should be reverted, the docs changes look better, though.

@ImRodry ImRodry changed the title refactor(ApplicationCommand): remove *Localized and better locale docs docs(ApplicationCommand): fix and improve localization docs Apr 17, 2022
@ImRodry ImRodry force-pushed the refactor/remove-localized branch 2 times, most recently from e8385ae to 033bd20 Compare April 17, 2022 11:47
@ImRodry
Copy link
Contributor Author

ImRodry commented Apr 17, 2022

That's inconsistent with other methods but if a maintainer wants that removed I'll remove it. I'll wait for now

Hi, maintainer here. The behaviour and defaults of the method were chosen internally, and as such, we took part of it.

IMO the code changes in this PR should be reverted, the docs changes look better, though.

I redid my tests and I'm now able to get the Localized properties so I have reverted the code changes and kept the documentation changes. Feel free to change the labels on this PR

@iCrawl iCrawl merged commit 61a44c5 into discordjs:main Apr 21, 2022
@ImRodry ImRodry deleted the refactor/remove-localized branch April 21, 2022 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

7 participants