search

rust-dc / fish-manpage-completions

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

Implement font, numreq, var, size, esc, word, text, and esc_char_backslash #68

Closed charliethomson closed 4 years ago

charliethomson commented 4 years ago

I also had to add stubs for comment, prch, text_arg, digit, condputs, and spec. I gave Deroffer::reg_table a type (HashMap<String, String>) based on the context it was used in. I think that's about it.

edit: v this is talking about #69 v Oh, I left commented out cleaner versions of a few lines that could be fixed with to change the interface of str_at and skip_char and probably also is_white, so yeah. Sorry for the PR spam Scott :P

scooter-dangle commented 4 years ago

We can also keep a reference to the original str, as part of the struct definition (e.g., struct Deroffer<'a> { output: Vec<Cow<'a, str>> /* Cow so that we can own if we need to */, s: &'a str, ... }). Not something we need to do now, though.

On Tue, Dec 10, 2019 at 10:06 AM Ivan Tham notifications@github.com wrote:

@pickfire commented on this pull request.

In src/deroff.rs https://github.com/rust-dc/fish-manpage-completions/pull/68#discussion_r356090053 :

  • "\" => {
  • // if self.str_at(1) == Some('"') {
  • if Self::str_at(&self.s, 1) == "\"" {
  • // self.condputs("\n");
  • return true;
  • }
  • }
  • "[" => {
  • self.refer = true;
  • // self.condputs("\n");
  • return true;
  • }
  • "]" => {
  • self.refer = false;
  • // self.skip_char();
  • self.s = self.skip_char(&self.s, None).to_owned();

Yeah, if any I believe we can still discuss on the design of parsing later on. A rough idea is to just keep the original String or &str and use an index to iterate through it.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rust-dc/fish-manpage-completions/pull/68?email_source=notifications&email_token=AAHEGM4NYXG5XP55MURJOT3QX6V6FA5CNFSM4JS2DCLKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCOU7CAA#discussion_r356090053, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHEGMYUMIPJDEZQLSFYR6LQX6V6FANCNFSM4JS2DCLA .

charliethomson commented 4 years ago

Is there anything I need to change on this PR?

pickfire commented 4 years ago

I think we can just take in this first? By the way, I feel like refactoring how string parsing current works.

charliethomson commented 4 years ago

I feel like refactoring how string parsing current works.

I definitely agree, most of the code is kinda begging for a reformat.

pickfire commented 4 years ago

Why is the original python code for this not removed? Is this still work in progress?

charliethomson commented 4 years ago

@pickfire I literally never look at the source in the file I'm editing, so I didn't even know it was there, they're removed now

pickfire commented 4 years ago

@charliethomson Can you please help to take a look at the rest of the comments?

EDIT: #75 is merged

scooter-dangle commented 4 years ago

@charliethomson, there are conflicts with master. Can you resolve?

pickfire commented 4 years ago

@charliethomson Please help to put the pull request as a draft, once you are done can ping me or make this a normal pull request so I can review one off. Thanks a lot.

charliethomson commented 4 years ago

@pickfire I fixed most things. There are a few I'm unsure about, I've replied to everything afaik. I'm not sure I understand the thing you pinged me in, can I get some clarification?