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

Use byte indices in Rust core #21

Open
bminixhofer opened this issue Feb 6, 2021 · 3 comments
Open

Use byte indices in Rust core #21

bminixhofer opened this issue Feb 6, 2021 · 3 comments
Assignees
Labels
enhancement New feature or request P3 Low Priority

Comments

@bminixhofer
Copy link
Owner

It would be more idiomatic to Rust to use byte indices everywhere internally (and everywhere in the public Rust API) and only convert to char indices at the boundary to Python.

@bminixhofer bminixhofer added enhancement New feature or request P3 Low Priority labels Feb 6, 2021
@bminixhofer bminixhofer self-assigned this Feb 6, 2021
@drahnr
Copy link
Contributor

drahnr commented Feb 11, 2021

I'd be in favour of providing both index bases, if they are already available internally. This avoids re-parsing bytes to characters on the user side.

@bminixhofer
Copy link
Owner Author

Do you actually use character indices or byte indices in cargo-spellcheck? Would you have to convert from byte indices to char indices if the Suggestion .start and .end indices were byte indices?

if they are already available internally.

For internal nlprule computation char indices are never needed (I think) so converting from char to byte as early as possible (i.e. when building the binaries) is possible. Using byte indices everywhere in Rust and only converting from byte to char at the boundary to Python made sense to me.

But you're right, it's worth thinking about providing both in the public API. I agree that that would make it nicer for a user. But for computation in nlprule I would like to be consistent in whether byte or char indices are used and ideally use bytes.

@drahnr
Copy link
Contributor

drahnr commented Feb 11, 2021

Do you actually use character indices or byte indices in cargo-spellcheck? Would you have to convert from byte indices to char indices if the Suggestion .start and .end indices were byte indices?

Yes, it's converted early on into character indices, and soon™ will be grapheme aware as well, but that can be a layer on-top of characters. It simplifies iterations significantly and is also required to properly align to spans provided by syn and ra iirc.

For internal nlprule computation char indices are never needed (I think) so converting from char to byte as early as possible (i.e. when building the binaries) is possible. Using byte indices everywhere in Rust and only converting from byte to char at the boundary to Python made sense to me.

I am just saying that having character based APIs is a nice feature, since it won't break with simple emojis which are multibyte characters.

But you're right, it's worth thinking about providing both in the public API. I agree that that would make it nicer for a user. But for computation in nlprule I would like to be consistent in whether byte or char indices are used and ideally use bytes.

👍 makes sense

@bminixhofer bminixhofer mentioned this issue Feb 26, 2021
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P3 Low Priority
Projects
None yet
Development

No branches or pull requests

2 participants