Code Review Request for a small lib called: hebrew_unicode_script

code review
1

Hi there,

I'm new to Rust and have written a small low level library called: hebrew_unicode_script.
I would like to receive feedback on the whole thing (set-up of the repository, documentation, test and the code itself) in order to improve.

Thanks in advance.

2

I would remove is-even as a dependency. It's just a dev-dependency, but there's no need to add an external crate for that functionality. just % 2 == 0 or add your own private trait if you really like being able to do .is_even().

2 Likes
3

is-even doesn't even do that. It outsources the work to another crate, is-odd and returns the negation of is-odd() !!

Nice educational crates by the way if you have never written a macro these ones are easy to read and understand.

4

Thank you very much.
The implementation is on GitHub v0.2.2

5

A more opinionated change might be to put all of your functions behind a trait.

This changes the API quite a bit, but could be more convenient depending on how you want to use the library.

For example you could do something like:

trait HebrewUnicodeScript {
    fn is_hbr_block(&self) -> bool;
    ...
}

impl HebrewUnicodeScript for char {
    fn is_hbr_block(&self) -> bool {
        // ...
    }
}

then the user could do:

use hebrew_unicode_script::HebrewUnicodeScript as _;
'מ'.is_hbr_block();

This is more of a API flavor preference as both would accomplish the same thing.

6

Thanx for the compliment.

About the macro stuff. I did nor realize that is-even was a macro. :slightly_smiling_face:
Writing a macro is somewhere at the end of my Rust learning todo list. Looking at is-even allthough it it only a few lines, it still seems a bit complicated for me now.

7

@richardscollin
I tried to figure things out regarding your proposal.
So far I got the following:

  • A Hebrew character is also UTF8 based, so this would fit the Rust char type?
  • My is_... fucntions look a lot like the ones at Rust char.
  • If I implement your proposal, then my code will be more like the the rust std and thus more familiar to other coders?

Q: Is your proposal a more idiomatic Rust version?

BTW I could not find were the char type is defined.

8

Yes, rust characters are a single unicode value (see the page you linked).

Is it more idiomatic? Maybe, I think it's debatable.

I personally think I prefer it better as a method because it matches the english grammar. The only way you can define methods on a char is by defining a trait.

I'm not usually a fan of when a crate I'm using has a trait I have to import. It's a bit annoying to have to remember the name of the trait, if it's different then the names of the methods you need to use.

The initial reason I thought of putting the methods on a trait is because in the code examples you were using a wildcard use statement to import all your functions. This is something that's convenient when writing code, but I think hurts readability in the long run. If you have many crates and modules all using wildcard imports you won't be able to easily read the code to determine the source of functions not defined in the file. (though easy with an editor configured to jump to definition)

Also:

    pub fn is_apf_consonant(c: char) -> bool {
        // An apf consonant can be any of the following:
        //  - an alternative consonant
        //  - a wide consonant
        //  - a consonant with a vowel
        match c {
            c if is_apf_consonant_alternative(c) => true,
            c if is_apf_consonant_wide(c) => true,
            c if is_apf_consonant_with_vowel(c) => true,
            _ => false,
        }
    }

Could be rewritten as:

    pub fn is_apf_consonant(c: char) -> bool {
         is_apf_consonant_alternative(c) || is_apf_consonant_wide(c) || is_apf_consonant_with_vowel(c)
    }
9

@richardscollin
Currently I am working on a program to search the Westminster Leningradus Codex.

At a point of time I had the idea of splitting the code in two parts: program specific and program a-specific.
The a-specific code could be outside my program as libs, so that others could be using those at well.
Meaning I am looking for the best/optimum usability of the library.

So far I have no experience in importing Traits.
Q. Why do you import Trait using "as _;"?

BTW Don't let my examples trick you. I just started coding in Rust!

PS
I will adapt the examples for readbility and modify the is_apf_consonant() when I'm home again :slightly_smiling_face:

10

The underscore in the trait import isn't required you could also just say use crate_name::TraitName;

The difference is the one without the underscore will bring the symbol TraitName into scope and the one will only let you use the trait without bringing the name into scope.

It's useful to use the underscore if the trait name is the same across crates or modules.

An example of this would be: std::io::Write and std::fmt::Write or serde::Serialize and rkyv::Serialize.

You can use the same syntax to bind a different symbol to the trait when you import it such as: use std::fmt::Write as FmtWrite.

11

@richardscollin
Thanks for the explanation.

Now there is still the question of usage of this library.

I got the following ideas:

Idea 1

Would it be possible to have the functions and the trait version in one library?
I just read that code will not be compiled (using --release) if code is not used.

Idea 2

Why not have two versions of this crate?

v1.0.0 : v0.2.0 + improvements as discussed
v2.0.0: all funcs behind a trait

In both cases the user can decide!

12

No need to have two library versions (idea 2). You're right you can have both styles of the api in the same crate (idea 1).

Yes the final binary shouldn't have extra code compiled in it if it's not used.

13

@richardscollin

It was good too get some feedback.
Thanks for the help!

I decided to make two API's available and put them in separate files.

1 Like
14

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.