Repository: sharkdp/bat
File: src/output.rs (Rust)

Code before:
use crate::wrapping::WrappingMode;

#[derive(Debug)]
pub enum OutputType {
    #[cfg(feature = "paging")]
    Pager(Child),
    Stdout(io::Stdout),
}

impl OutputType {
    #[cfg(feature = "paging")]
    pub fn from_mode(paging_mode: PagingMode, wrapping_mode: WrappingMode, pager: Option<&str>) -> Result<Self> {
        use self::PagingMode::*;
        use self::WrappingMode::*;
        Ok(match paging_mode {
            Always => OutputType::try_pager(false, wrapping_mode == Character, pager)?,
            QuitIfOneScreen => OutputType::try_pager(true, wrapping_mode == Character, pager)?,
            _ => OutputType::stdout(),
        })
    }

    /// Try to launch the pager. Fall back to stdout in case of errors.
    #[cfg(feature = "paging")]
    fn try_pager(
        quit_if_one_screen: bool,
        line_wrap: bool,
        pager_from_config: Option<&str>
    ) -> Result<Self> {
        use std::env;
        use std::ffi::OsString;
        use std::path::PathBuf;
        use std::process::{Command, Stdio};

        let mut replace_arguments_to_less = false;

        let pager_from_env = match (env::var("BAT_PAGER"), env::var("PAGER")) {
            (Ok(bat_pager), _) => Some(bat_pager),
            (_, Ok(pager)) => {
                // less needs to be called with the '-R' option in order to properly interpret the
                // ANSI color sequences printed by bat. If someone has set PAGER="less -F", we
                // therefore need to overwrite the arguments and add '-R'.
                //
                // We only do this for PAGER (as it is not specific to 'bat'), not for BAT_PAGER
                // or bats '--pager' command line option.
                replace_arguments_to_less = true;
                Some(pager)
            }
            _ => None,
        };

        let pager_from_config = pager_from_config.map(|p| p.to_string());

Review comment (suggestion):
As we don't have named parameters in Rust, I really like to avoid `bool` parameters. One is bad enough already (*"What does `OutputType::try_pager(false, pager)` do?"*), but two are definitely not ideal (I know that one was here already). It would be great if we could introduce two `enum`s instead. For example:
``` rust
enum ActionForShortFiles {
  QuitPager,
  Nothing,
}
```
or maybe just use the `less` wording with something like
``` rust
enum ChopLongLines {
  Yes,
  No,
}
```

This is a small point overall, but I think it would be great to improve this.

Code after:
use crate::wrapping::WrappingMode;


#[cfg(feature = "paging")]
#[derive(Debug, PartialEq)]
enum SingleScreenAction {
    Quit,
    Nothing,
}


#[derive(Debug)]
pub enum OutputType {
    #[cfg(feature = "paging")]
    Pager(Child),
    Stdout(io::Stdout),
}

impl OutputType {
    #[cfg(feature = "paging")]
    pub fn from_mode(paging_mode: PagingMode, wrapping_mode: WrappingMode, pager: Option<&str>) -> Result<Self> {
        use self::PagingMode::*;
        Ok(match paging_mode {
            Always => OutputType::try_pager(SingleScreenAction::Nothing, wrapping_mode, pager)?,
            QuitIfOneScreen => OutputType::try_pager(SingleScreenAction::Quit, wrapping_mode, pager)?,
            _ => OutputType::stdout(),
        })
    }

    /// Try to launch the pager. Fall back to stdout in case of errors.
    #[cfg(feature = "paging")]
    fn try_pager(
        single_screen_action: SingleScreenAction,
        wrapping_mode: WrappingMode,
        pager_from_config: Option<&str>
    ) -> Result<Self> {
        use std::env;
        use std::ffi::OsString;
        use std::path::PathBuf;
        use std::process::{Command, Stdio};

        let mut replace_arguments_to_less = false;

        let pager_from_env = match (env::var("BAT_PAGER"), env::var("PAGER")) {
            (Ok(bat_pager), _) => Some(bat_pager),
            (_, Ok(pager)) => {
                // less needs to be called with the '-R' option in order to properly interpret the
                // ANSI color sequences printed by bat. If someone has set PAGER="less -F", we
                // therefore need to overwrite the arguments and add '-R'.
                //
                // We only do this for PAGER (as it is not specific to 'bat'), not for BAT_PAGER