Skip to content

Conversation

m-aciek
Copy link
Contributor

@m-aciek m-aciek commented Dec 9, 2023

Raising ValueErrors with descriptive error messages.

  • It handles empty Plural-Forms like "Plural-Forms: \n" (original downstream bug, and fix) but also lack of plural= keyword, like "Plural-Forms: nplurals=4;\n".
    • ValueError: expected ';' and 'plural=' in Plural-Forms metadata in sample.mo, got ''
    • previously IndexError
  • It handles incorrect Content-Type
    • ValueError: expected 'charset=' in Content-Type metadata in sample.mo, got ''
    • previously IndexError

In the #56634 ignoring the incorrect syntax was being discussed, as well as issuing warnings.

The implementation here is following the suggestion of @serhiy-storchaka of raising an error instead.

@m-aciek m-aciek force-pushed the gettext-handle-incorrect-plural-forms branch from a2515c9 to 17baf50 Compare December 9, 2023 11:33
@m-aciek m-aciek changed the title gh-56634: Gettext: handle gracefully empty plural-forms value gh-56634: Gettext: raise descriptive error on empty plural-forms value Dec 9, 2023
@m-aciek m-aciek force-pushed the gettext-handle-incorrect-plural-forms branch from 17baf50 to e38c34c Compare December 9, 2023 11:35
@m-aciek m-aciek force-pushed the gettext-handle-incorrect-plural-forms branch from e38c34c to e148f83 Compare December 9, 2023 11:37
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Could you please add similar change for content-type?

@m-aciek m-aciek changed the title gh-56634: Gettext: raise descriptive error on empty plural-forms value gh-56634: Gettext: raise descriptive errors during .mo files metadata parsing Sep 25, 2025
@m-aciek m-aciek changed the title gh-56634: Gettext: raise descriptive errors during .mo files metadata parsing gh-56634: Gettext: raise descriptive ValueErrors instead of IndexErrors for incorrect .mo files metadata Sep 25, 2025
@m-aciek
Copy link
Contributor Author

m-aciek commented Sep 25, 2025

  • I've covered content type metadata, as requested.
  • Changed error messages to:
    • ValueError: expected ';' and 'plural=' in Plural-Forms metadata in sample.mo, got ''
    • and ValueError: expected 'charset=' in Content-Type metadata in sample.mo, got ''
  • Added a test for second case of plural forms and for content type error.

@serhiy-storchaka Would it be possible for you to find a spare moment for a re-review? Thank you!

@m-aciek
Copy link
Contributor Author

m-aciek commented Sep 26, 2025

(Fixed Windows support in tests.)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants