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

chore(i18n): change localization attributes' defaults #1866

Merged

Conversation

Middledot
Copy link
Member

@Middledot Middledot commented Jan 11, 2023

Summary

EDIT: This PR makes it so that l10n attrs are by default MISSING to be more API consistent.
I'd call this a minor breaking change.

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.

@Middledot Middledot requested a review from a team as a code owner January 11, 2023 03:19
@codecov
Copy link

codecov bot commented Jan 11, 2023

Codecov Report

❗ No coverage uploaded for pull request base (master@c43727b). Click here to learn what that means.
The diff coverage is 6.25%.

❗ Current head 9db28a4 differs from pull request most recent head e45f280. Consider uploading reports for the commit e45f280 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1866   +/-   ##
=========================================
  Coverage          ?   33.27%           
=========================================
  Files             ?       97           
  Lines             ?    19022           
  Branches          ?        0           
=========================================
  Hits              ?     6329           
  Misses            ?    12693           
  Partials          ?        0           
Flag Coverage Δ
macos-latest-3.10 33.25% <6.25%> (?)
macos-latest-3.11 33.25% <6.25%> (?)
macos-latest-3.8 33.26% <6.25%> (?)
macos-latest-3.9 33.26% <6.25%> (?)
ubuntu-latest-3.10 33.25% <6.25%> (?)
ubuntu-latest-3.11 33.25% <6.25%> (?)
ubuntu-latest-3.8 33.26% <6.25%> (?)
ubuntu-latest-3.9 33.26% <6.25%> (?)
windows-latest-3.10 33.25% <6.25%> (?)
windows-latest-3.11 33.25% <6.25%> (?)
windows-latest-3.8 33.26% <6.25%> (?)
windows-latest-3.9 33.26% <6.25%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
discord/commands/core.py 17.99% <0.00%> (ø)
discord/commands/options.py 17.12% <16.66%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c43727b...e45f280. Read the comment docs.

@Middledot Middledot changed the title Change localization attributes' defaults chore(i18n): change localization attributes' defaults Jan 11, 2023
@BobDotCom
Copy link
Member

IMO, to better reflect the API it should actually default to MISSING. We omit the field (hence "missing") when localizations are not set, we don't send None, nor do we send {}.

@Middledot
Copy link
Member Author

Well, the difference is that using an empty dictionary would remove the redundancy of needing set the dictionary before or while adding a translation and using MISSING would be more API consistent. Personally I made this pr because I found it weird that I had to that

@BobDotCom
Copy link
Member

Still not convinced that this change is really necessary IMO

@Lulalaby
Copy link
Member

Tbh agreeing with bob

@Middledot
Copy link
Member Author

Mmmh, ok, I'll do that

@Middledot Middledot requested review from Lulalaby and BobDotCom April 2, 2023 08:42
discord/commands/core.py Outdated Show resolved Hide resolved
discord/commands/core.py Outdated Show resolved Hide resolved
adapt

Co-authored-by: plun1331 <[email protected]>
Signed-off-by: Middledot <[email protected]>
@Middledot
Copy link
Member Author

I'm waiting for #1867 to be merged first before I update the changelog for this one

@Lulalaby Lulalaby enabled auto-merge (squash) May 8, 2023 14:40
@Lulalaby
Copy link
Member

Lulalaby commented May 8, 2023

@Middledot could you in future pls work on a branch.. codeconv is shitting itself bcs fork .-.

@Lulalaby Lulalaby changed the base branch from master to i18n May 8, 2023 15:09
@Lulalaby Lulalaby disabled auto-merge May 8, 2023 15:09
@Lulalaby Lulalaby merged commit a58570a into Pycord-Development:i18n May 8, 2023
@Middledot Middledot deleted the localization-attr-defaults branch May 8, 2023 15:23
Lulalaby added a commit that referenced this pull request May 8, 2023
* chore(i18n): make defaults for l10ns an empty dict

* chore(changelog): add entry

* fix(changlog): update entry

oops

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* feat(i18n): None -> MISSING

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Apply suggestions from code review

adapt




---------

Signed-off-by: Middledot <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: plun1331 <[email protected]>
Co-authored-by: Lala Sabathil <[email protected]>
@pullapprove4
Copy link

pullapprove4 bot commented Jul 27, 2023

Please add a changelog entry

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.

4 participants