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.

Sign up for GitHub

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

Jump to bottom

🐛 Fix message reporting breaking on empty string #1155

Merged
merged 7 commits into from
May 20, 2024
Merged

🐛 Fix message reporting breaking on empty string #1155

merged 7 commits into from
May 20, 2024

Conversation

NeunEinser
Copy link
Contributor

@NeunEinser NeunEinser commented May 17, 2024 •
edited
Loading

When sending an empty string to error reporting, no errors show up in the entire file.

This fixes this in two ways:

  • When trying to send an empty string to b reported, it's just ignored and no error is reported for that line
  • When trying to localize an unknown key, instead of returning an empty string, the key itself is returned
    • This could have side effects in other code that relies on a broken key to return nothing. That seems like a potential bug though, so I think highlighting that with showing the translation key is not bad.
    • Potentially, we could also have it depend on some sort of 'dev' flag which to not show keys in a release build
    • When developing, this illustrates the kind of mistake you made better. Instead of not showing an error, which may lead you thinking your error reporting logic is somehow wrong, you immediately see that you are missing a translation key

@NeunEinser NeunEinser requested a review from SPGoding as a code owner May 17, 2024 11:18
@NeunEinser NeunEinser force-pushed the feature/fix-empty-string-error-reporting branch from c04846d to 4b02fde Compare May 19, 2024 15:21
@NeunEinser
Copy link
Contributor Author

NeunEinser commented May 19, 2024 •
edited
Loading

Looks like there are some missing translation keys for longs, as illustrated by the test changes. I can add them in this PR, I think it would make sense?

Edit: I added those missing strings

@misode misode merged commit a1f81c3 into SpyglassMC:main May 20, 2024
3 checks passed
TheAfroOfDoom pushed a commit to TheAfroOfDoom/Spyglass that referenced this pull request May 24, 2024
@NeunEinser @TheAfroOfDoom
🐛 Fix message reporting breaking on empty string (SpyglassMC#1155)
765bc9d
TheAfroOfDoom added a commit to TheAfroOfDoom/Spyglass that referenced this pull request May 24, 2024
@TheAfroOfDoom
Squashed commit of the following:
d3705e0 
commit a8431ea
Author: SPGoding <i@spgoding.com>
Date:   Thu May 23 16:54:49 2024 -0500

    🐛 Fix computing relative URIs (SpyglassMC#1177)

commit 0c5e505
Author: Afro <tehafroofdoom@gmail.com>
Date:   Thu May 23 17:53:36 2024 -0400

    🔧 Add `import/no-duplicates` lint rule (SpyglassMC#1181)

commit 50c2185
Author: SPGoding <i@spgoding.com>
Date:   Wed May 22 21:26:34 2024 -0500

    🔥 Remove CommandTreeRegistry (SpyglassMC#1190)

commit 6439d5f
Author: NeunEinser <daniel30797@gmail.com>
Date:   Thu May 23 04:24:54 2024 +0200

    🚧 Remove attributed type in favor of attributes array on all types (SpyglassMC#1182)

commit d99eeb6
Author: SPGoding <i@spgoding.com>
Date:   Wed May 22 21:22:37 2024 -0500

    ⬆️ Upgrade dprint (SpyglassMC#1191)

commit 9a303cc
Author: Afro <tehafroofdoom@gmail.com>
Date:   Wed May 22 22:21:45 2024 -0400

    🔧 Add long timeout arg to unit test launch configuration (SpyglassMC#1189)

commit 2b74e1f
Author: Nico314159 <nicolino.will@gmail.com>
Date:   Wed May 22 13:04:25 2024 -0700

    Implement 1.20.5+ item slots (SpyglassMC#1179)

commit d6c395f
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue May 21 02:58:56 2024 -0500

    ⬆️ Bump rexml from 3.2.5 to 3.2.8 in /docs (SpyglassMC#1153)

    Signed-off-by: dependabot[bot] <support@github.com>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit a1f81c3
Author: NeunEinser <daniel30797@gmail.com>
Date:   Mon May 20 22:15:26 2024 +0200

    🐛 Fix message reporting breaking on empty string (SpyglassMC#1155)
@NeunEinser NeunEinser deleted the feature/fix-empty-string-error-reporting branch May 25, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@misode misode misode approved these changes

@SPGoding SPGoding SPGoding approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NeunEinser @SPGoding @misode