Skip to content

Conversation

LilyFirefly
Copy link
Contributor

Unix filepaths are not necessarily utf-8, so representing them with Python str or Rust String isn't ideal. Python's pathlib handles this, as does Rust's PathBuf and PyO3 knows how to convert between them.

@LilyFirefly LilyFirefly marked this pull request as ready for review August 4, 2025 08:31
@LilyFirefly LilyFirefly requested a review from a team as a code owner August 4, 2025 08:31
@LilyFirefly LilyFirefly force-pushed the use-pathbuf branch 3 times, most recently from 93647c7 to b05f3b9 Compare August 4, 2025 08:43
@LilyFirefly LilyFirefly self-assigned this Aug 4, 2025
@LilyFirefly LilyFirefly added enhancement New feature or request rust Pull requests that update rust code labels Aug 4, 2025
Copy link
Collaborator

@seddonym seddonym left a comment

Choose a reason for hiding this comment

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

Looks good but I think we should make sure there's test coverage for string inputs.

As this is a user-facing change, could we also update the README and CHANGELOG please?



def test_en_basic():
bundle = fluent.Bundle("en", [str(data_dir / "en.ftl")])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we have a test that checks that strings are still supported?

This is a bit more permissive on unix systems, where filenames don't
have to be `utf-8`.
Copy link
Collaborator

@seddonym seddonym left a comment

Choose a reason for hiding this comment

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

Thanks!

@seddonym seddonym merged commit 72eefa5 into kraken-tech:main Aug 4, 2025
1 check passed
@LilyFirefly LilyFirefly deleted the use-pathbuf branch August 4, 2025 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rust Pull requests that update rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants