search

marty-oehme / bemoji

Emoji picker that remembers your favorites, with support for bemenu/wofi/rofi/dmenu and wayland/X11.
MIT License
106 stars 6 forks source link

✨ Add -n option to prevent newline after emoji #12

Closed nobodyinperson closed 2 years ago

nobodyinperson commented 2 years ago

This Merge Request add the bemoji -n option to not include a newline character after the selected emoji.

A newline after the emoji might or might not be wanted, which this option the user can choose.

In my case I don't want the newline as pasting it into xfce4-terminal a warning is shown whenever pasting something with newlines in it.

nobodyinperson commented 2 years ago

I also tried to add a BATS test for this new -n option. But funnily enough, BATS does not seem to be designed for this: assert_output ignores the trailing newline and assert_output --regex '^❤️$' fails for some reason 🤷

nobodyinperson commented 2 years ago

As for the default behaviour: I'd rather keep bemoji backwards-compatible and make -n opt-in as it is in this implementation. Although I can't imagine a situation where one would want a trailing newline either 😅

marty-oehme commented 2 years ago

I also tried to add a BATS test for this new -n option. But funnily enough, BATS does not seem to be designed for this: assert_output ignores the trailing newline and assert_output --regex '^❤️$' fails for some reason 🤷

This is a good point, I remember having trouble with the bats regexes in some other tests. You probably have this correct but just wanna point it out nonetheless, the option is --regexp.

Anyway, I think it's an option that would be really worth to have a little test for, I'll also see how we could implement it over the coming days!

marty-oehme commented 2 years ago

Well, after quite a bit of fiddling I managed to make the tests work. Turns out we have to invoke the tests with run --keep-empty-lines for it to work. This seems to be due to an old bug in bats which is now taken care of with the commandline option.

This now looks good as is - though I'm still not fully convinced of the necessity to keep the old behavior if we both can't think of any newline containing use case.

marty-oehme commented 2 years ago

I added some quick documentation.

@nobodyinperson I think this looks good now and if you agree I'm finally ready to merge! :)

nobodyinperson commented 2 years ago

Looks good to me 👍