rust-dc / fish-manpage-completions

Straight port of fish-shell's Python manpage completion script to Rust
19 stars 8 forks source link

impl `condputs` #64

Closed charliethomson closed 4 years ago

charliethomson commented 4 years ago

I combined condputs and condputs_tr, because one overloads the other later on. I also changed the type of Deroffer:self.tr from String to Option<HashMap<char, char>> in order to be able to interface with util:translate and util:maketrans. In addition, I set the type of Deroffer:self.output to String

pickfire commented 4 years ago

Any news on this?

scooter-dangle commented 4 years ago

Aside from the comments above, looks good overall.

It's not strictly necessary, but you might consider making the translation table a dedicated type (as @kbknapp had done in #56). If so, it might make sense to convert maketrans to TranslateTable::new, e.g.,

impl TranslateTable {
    pub fn new(f: &str, t: &str) -> Result<Self, String> {
        /* ... */
    }
    /* ... */
}
scooter-dangle commented 4 years ago

Note: See additional discussion/review of some of the code in this PR in #62

scooter-dangle commented 4 years ago

Oh, also. Could you delete the Python source for condputs and condputs_tr as part of this branch?

charliethomson commented 4 years ago
pub struct TranslationTable {
    table: HashMap<char, char>,
} impl TranslationTable {
    pub fn new(f: &str, t: &str) -> Result<Self, &'static str> {
        if f.len() != t.len() {
            Err("Arguments passed to maketrans must have equal length")
        } else {
            Ok(TranslationTable { table: HashMap::from_iter(f.chars().zip(t.chars())) })
        }
    }

    pub fn translate(&self, s: String) -> String {
        s
            .chars()
            .map(|c| *self.table.get(&c).unwrap_or(&c))
            .collect::<String>()
    }
}

Thoughts?

scooter-dangle commented 4 years ago
pub struct TranslationTable {
    table: HashMap<char, char>,
} impl TranslationTable {
    pub fn new(f: &str, t: &str) -> Result<Self, &'static str> {
        if f.len() != t.len() {
            Err("Arguments passed to maketrans must have equal length")
        } else {
            Ok(TranslationTable { table: HashMap::from_iter(f.chars().zip(t.chars())) })
        }
    }

    pub fn translate(&self, s: String) -> String {
        s
            .chars()
            .map(|c| *self.table.get(&c).unwrap_or(&c))
            .collect::<String>()
    }
}

Thoughts?

Very close. You'll want to check character length instead of byte length, since we're operating on characters. (You can use the char_len function in main.rs for that.)

Also, you'll want to tweak the error message to be

"Arguments passed to `TranslationTable::new` must have equal character length"
charliethomson commented 4 years ago

Donezo, anything else?