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

Fix for Issue 778. #779

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

oussjarrousse
Copy link

Summary of the changes in this pull request

  • Added rednotebook.utils.safe_strxfrm() fixing issue-778 and updated rednotebook.journal.Journal.categories() to use utils.safe_strxfrm() in sorting categories instead of locale.strxfrm() that was raising OSError in case of characters incompatible with the current locale
  • Added tests/test_journal.py that contains a the function test_categories() that tests journal.categories including the specific case of issue-778.

Pull request checklist

  • I have added an entry in CHANGELOG.md including my name and issue and/or pull request number.
  • [] If applicable: I have removed the corresponding entry in TODO.md.

…ebook.journal.Journal.categories() to use utils.safe_strxfrm in sorting categories instead of locale.strxfrm that was raising OSError in case of characters incompatible with the current locale; added tests.test_journal.py to test the fix for issue-778
Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this!

@@ -1,3 +1,6 @@
# 2.36 (2024-10-24)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
# 2.36 (2024-10-24)
# 2.36 (unreleased)

@@ -1,3 +1,6 @@
# 2.36 (2024-10-24)
* Fix: Categories with characters incompatible with the current locale opens (#778, Oussama Jarrousse).
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
* Fix: Categories with characters incompatible with the current locale opens (#778, Oussama Jarrousse).
* Fix sorting words that are incompatible with the current locale (#778, #779, Oussama Jarrousse).

@@ -202,3 +203,14 @@ def flush(self):
def close(self):
for stream in self.streams:
stream.close()

def safe_strxfrm(value):
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
def safe_strxfrm(value):
def safe_strxfrm(s: str):

return locale.strxfrm(value)
except OSError:
return value
except:
Copy link
Owner

Choose a reason for hiding this comment

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

Do we really need the bare except? I think it's better to remove it since we have the except for OSError.

Copy link
Owner

Choose a reason for hiding this comment

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

Nice test!

@jendrikseipp
Copy link
Owner

You can ignore the failing Mac test. But please see that the other tests pass.

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.

2 participants