glow icon indicating copy to clipboard operation
glow copied to clipboard

fix: handle PAGER paths with spaces

Open Shreya-7 opened this issue 2 years ago • 8 comments

fixes #385

This fixes the handling of pager paths pulled from environment variables. It works on the assumption that the value of the PAGER environment variable will be the path to an executable, and not an actual command (with arguments and what not).

The logic has been moved inside a utility function for the following reasons:

  • If, in the future, we need to support more complex logic for building the actual command to be run for using pagers, we can add it in this function instead of bloating our main code.
  • Makes the code re-usable, if we need to support such logic for other options (after renaming).
  • Makes it easier to test.

Shreya-7 avatar Oct 09 '22 13:10 Shreya-7

@bashbunni do you mind reviewing this PR?

Shreya-7 avatar Oct 09 '22 13:10 Shreya-7

@muesli I see you added the bug label to this PR. Silly question, but is it because it solves a bug, or are the changes this PR introduced not working as expected? 😐

Shreya-7 avatar Oct 17 '22 02:10 Shreya-7

It's there to indicate it addresses a bug in glow.

muesli avatar Oct 17 '22 02:10 muesli

@muesli do the changes look good to you? Do we need something else as well here?

Shreya-7 avatar Oct 23 '22 17:10 Shreya-7

Looks pretty good already, see the two comments I left tho. Extract credits for adding a test case! :+1:

muesli avatar Oct 23 '22 18:10 muesli

@muesli do you mind giving this a look again?

Shreya-7 avatar Oct 25 '22 18:10 Shreya-7

Sorry for the delay @Shreya-7 we will get to it, there is just some pending functionality we need to figure out before merging

bashbunni avatar Jan 09 '23 16:01 bashbunni

@bashbunni do we still need this bugfix PR?

Shreya-7 avatar Jul 20 '23 16:07 Shreya-7