New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the core.pager config for overriding the system PAGER. #329

Merged
merged 4 commits into from May 2, 2017

Conversation

Projects
None yet
3 participants
@yati-sagade
Contributor

yati-sagade commented Apr 21, 2017

As discussed in #328, this PR adds support for the core.pager setting prop. I thought about what to do when the config file ends up having multiple values for the pager property, and IMO dying would be a bit extreme (imagine sqitch log failiing with Multiple values for key "core.pager"). Happy to change it if we think some other strategy would be appropriate.

This is my CPAN PRC April 2017 assignment.

yati-sagade added some commits Apr 21, 2017

configuration: Add the App'Sqitch'Config'get_last_value method.
When a property has multiple values in the config file, but we don't
want to error out just because, one of the sane things to do could be to
get the last value in the config for the property.

An example of such a "non-essential" property could be the pager program
to use, core.pager. If it has multiple values set, we likely don't want
to bail out, but just do a best effort pick by using the last value for
the property.
config: Recognize the core.pager property as the pager program to use…
… for all commands.

Commands like `sqitch log` output ANSI color codes when a capable
terminal is detected. By itself, this output shows fine on most terminal
emulators, but when piped to, say, `less`, the output can include the
escaped color codes instead of colored text, depending on the system.
Now IO::Pager looks for the PAGER environment variable, and the user
could just set that to, say, `less -r` to properly show colors. This
patch just adds support for a sqitch specific pager program, specified
by the core.pager property.

If %ENV does not contain "PAGER", this patch sets it to undef, which
works with IO::Pager, although it is not technically the same (!exists
$ENV{PAGER} is different than !$ENV{PAGER}). If we _really_ need to
preserve the existence/non-existence of keys while localizing, we can do
something like

    goto JMP unless exists $ENV{PAGER};
    local $ENV{PAGER} = $config->get_last_value(key => "core.pager");
    JMP:
    ...

But fortunately we don't need that yet.
@theory

This comment has been minimized.

Show comment
Hide comment
@theory

theory Apr 21, 2017

Collaborator

Hrm. This is good, but looking for precedents, I think the handling of the EDITOR environment variable is the one to follow. To quote changes from the last release:

- Fixed editor selection to prioritize the `core.editor` configuration
  variable over the `$EDITOR` environment variable. The `$SQITCH_EDITOR`
  environment variable still trumps all. Thanks to Jim Nasby for the pull
  request (#296).

You can see the precedence defined here. Can you update your PR to follow that same pattern, adding a SQITCH_PAGER environment variable?

Collaborator

theory commented Apr 21, 2017

Hrm. This is good, but looking for precedents, I think the handling of the EDITOR environment variable is the one to follow. To quote changes from the last release:

- Fixed editor selection to prioritize the `core.editor` configuration
  variable over the `$EDITOR` environment variable. The `$SQITCH_EDITOR`
  environment variable still trumps all. Thanks to Jim Nasby for the pull
  request (#296).

You can see the precedence defined here. Can you update your PR to follow that same pattern, adding a SQITCH_PAGER environment variable?

configuration: Add support for the SQITCH_PAGER environment variable.
In line with the SQITCH_EDITOR variable, this will trump all the pager
settings, in effect making sqitch search for the pager program to use in
the following order:

1. SQITCH_PAGER environment variable.
2. core.pager configuration setting.
3. PAGER environment variable.

When none of these are set, we just let IO::Pager do it's thing, which
at the moment is to find the common pagers in their common install
locations, and failing all else, "Set PAGER to more and cross our
fingers".
@yati-sagade

This comment has been minimized.

Show comment
Hide comment
@yati-sagade

yati-sagade Apr 22, 2017

Contributor

Done! :)

Contributor

yati-sagade commented Apr 22, 2017

Done! :)

@theory

This comment has been minimized.

Show comment
Hide comment
@theory

theory Apr 23, 2017

Collaborator

This looks great. However, in no other place has the code bothered to worry about whether a config key has multiple values. For example, the editor attribute just looks like this:

default => sub {
    return
         $ENV{SQITCH_EDITOR}
      || shift->config->get( key => 'core.editor' )
      || $ENV{VISUAL}
      || $ENV{EDITOR}
      || ( $^O eq 'MSWin32' ? 'notepad.exe' : 'vi' );
}

So do we really need the get_last_value method? Or should that perhaps be considered a better way to always fetch a single value? Or maybe there should be a config method to complain if a key has more than one value?

I realize that's all orthogonal to the raison d'être for this patch, and I'm happy to merge it either way. If we do decide to keep get_last_value, you should document it in order to get the tests passing again.

Collaborator

theory commented Apr 23, 2017

This looks great. However, in no other place has the code bothered to worry about whether a config key has multiple values. For example, the editor attribute just looks like this:

default => sub {
    return
         $ENV{SQITCH_EDITOR}
      || shift->config->get( key => 'core.editor' )
      || $ENV{VISUAL}
      || $ENV{EDITOR}
      || ( $^O eq 'MSWin32' ? 'notepad.exe' : 'vi' );
}

So do we really need the get_last_value method? Or should that perhaps be considered a better way to always fetch a single value? Or maybe there should be a config method to complain if a key has more than one value?

I realize that's all orthogonal to the raison d'être for this patch, and I'm happy to merge it either way. If we do decide to keep get_last_value, you should document it in order to get the tests passing again.

config: Ignore multiple values for the core.pager config
In my previous commits which implemented the core.pager config, we were
handling multiple config values by getting the last value for the
property. But when John pointed out that we don't really care that much
about this for other properties, it seems about reasonable to keep the
code simple at the expense of a potential "multiple values" error
message, which would hopefully prompt the user to check their config
file any way. So removing App::Sqitch::Config->get_last_value and its
uses.
@yati-sagade

This comment has been minimized.

Show comment
Hide comment
@yati-sagade

yati-sagade Apr 27, 2017

Contributor

Hey, I think you're right about the orthogonality of these two issues, and I've updated the PR to just use $config->get, like for core.editor. I think a separate PR for warning when multiple values are present, while addressing this for all config keys makes more sense. It can be as simple as rigging App'Sqitch'Config to catch the error from Config'Gitlike and then die with a more descriptive error message, pointing the user to the faulty config file + line.

Contributor

yati-sagade commented Apr 27, 2017

Hey, I think you're right about the orthogonality of these two issues, and I've updated the PR to just use $config->get, like for core.editor. I think a separate PR for warning when multiple values are present, while addressing this for all config keys makes more sense. It can be as simple as rigging App'Sqitch'Config to catch the error from Config'Gitlike and then die with a more descriptive error message, pointing the user to the faulty config file + line.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 27, 2017

Coverage Status

Coverage increased (+0.002%) to 90.804% when pulling 5aa99f6 on yati-sagade:master into dc5e2a8 on theory:master.

coveralls commented Apr 27, 2017

Coverage Status

Coverage increased (+0.002%) to 90.804% when pulling 5aa99f6 on yati-sagade:master into dc5e2a8 on theory:master.

@yati-sagade yati-sagade reopened this Apr 29, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 2, 2017

Coverage Status

Coverage increased (+0.002%) to 90.804% when pulling 5aa99f6 on yati-sagade:master into dc5e2a8 on theory:master.

coveralls commented May 2, 2017

Coverage Status

Coverage increased (+0.002%) to 90.804% when pulling 5aa99f6 on yati-sagade:master into dc5e2a8 on theory:master.

@theory theory merged commit a936e4a into sqitchers:master May 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 90.804%
Details
@theory

This comment has been minimized.

Show comment
Hide comment
@theory

theory May 2, 2017

Collaborator

Thanks!

Collaborator

theory commented May 2, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment