Changes to the interfaces for Deroffer str_at and skip_char to not be so ugly

[charliethomson]

I think we should make str_at a method instead of an associated function, as well as, for str_at and skip_char, stop taking a &str argument and just modify self.s within the function. I also think we should stop taking an Option<usize> on skip_char and just take a usize (from what I can gather, the Option is only used because the python uses a default arg and it wants to mirror that, however I feel that its clearer to just take a number)

Problem

if let Some("dy") = self.s.get(3..5) {
    // This
    self.s = self.skip_char(self.s.as_str(), Some(5)).to_owned();
    return true;
} else if Self::str_at(self.s.as_str(), 2) == "(" && self.prch(3) && self.prch(4) {
    // This
    self.s = self.skip_char(self.s.as_str(), Some(5)).to_owned();
    return true;
} else if Self::str_at(self.s.as_str(), 2) == "]" && self.prch(3) {
    // This
    self.s = self.skip_char(self.s.as_str(), Some(3)).to_owned();
    // This
    let mut s0 = Self::str_at(self.s.as_str(), 0);
    while !s0.is_empty() && s0 != "]" {
        // This
        self.s = self.skip_char(self.s.as_str(), None).to_owned();
        // This
        s0 = Self::str_at(self.s.as_str(), 0);
    }
    return true;
}

Current interface

Deroffer::str_at(string: &str, idx: usize) -> &str

Deroffer::skip_char(&self, s: &str, amount: Option<usize>) -> &str

Proposed change

Deroffer::str_at(&self, idx: usize) -> Option<char>

Deroffer::skip_char(&mut self, amount: usize) -> ()

after

if let Some("dy") = self.s.get(3..5) {
    // self.s = self.skip_char(self.s.as_str(), Some(5)).to_owned();
    self.skip_char(5);
    return true;
} else if Self::str_at(self.s.as_str(), 2) == "(" && self.prch(3) && self.prch(4) {
    // self.s = self.skip_char(self.s.as_str(), Some(5)).to_owned();
    self.skip_char(5);
    return true;
} else if Self::str_at(self.s.as_str(), 2) == "]" && self.prch(3) {
    // self.s = self.skip_char(self.s.as_str(), Some(3)).to_owned();
    self.skip_char(3);
    // let mut s0 = Self::str_at(self.s.as_str(), 0);
    while let Some(s0) = self.str_at(0) {
        if s0 == ']' { break; }
        // self.s = self.skip_char(self.s.as_str(), None).to_owned();
        // the value if it's passed None is 1 
        self.skip_char(1);
        // s0 = Self::str_at(self.s.as_str(), 0);
        s0 = self.str_at(0);
    }
    return true;
}

Thoughts? There may be a reason it's designed like that but it makes using the interface verbose, tedious, and ugly.

[necaris]

@charliethomson I seem to recall one of the light guiding principles we had when we started working on this was to do a faithful port of all the Python logic, warts and all, and get it working and passing tests before we changed things around to make it more idiomatic.

I don't know if that's something we still want to hold on to, but I seem to recall us thinking the original code was hard enough to grok as-is without also having to translate the idioms.

[charliethomson]

The interface would mirror the Python version better and be more idiomatic if we implemented these changes.

Here's the original source for the two functions:

def skip_char(self, amt=1):
    self.s = self.s[amt:]

def str_at(self, idx):
        return self.s[idx : idx + 1]

skip_char modifies (&mut self) self.s to skip forward by amt (currently Option<usize>, proposing usize) characters str_at takes what in rust would be a &self and an index (usize) and returns the character (Option<char>) at the index

When compared to the original source, I think the proposed change not only makes the interface cleaner, but it makes it mirror the python better

[pickfire]

Yes, if we want to mirror the original API, it should be done that way. I was thinking of this but feels weird it is implemented the current way instead of being similar to python.