nlprule icon indicating copy to clipboard operation
nlprule copied to clipboard

Use byte indices in Rust core

Open bminixhofer opened this issue 3 years ago • 3 comments

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 avatar Feb 06 '21 12:02 bminixhofer

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.

drahnr avatar Feb 11 '21 14:02 drahnr

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.

bminixhofer avatar Feb 11 '21 16:02 bminixhofer

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.

:+1: makes sense

drahnr avatar Feb 11 '21 17:02 drahnr