Skip to content
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

Add new show command #43

Merged
merged 2 commits into from
Jul 25, 2019
Merged

Conversation

julen
Copy link
Contributor

@julen julen commented Oct 26, 2016

When configurations use inheritance, it is handy to see the expanded
configuration to get a better feel of what is going to be run. While
Config::Neat already provides a command for this, having a show
command helps with the feature being discoverable.

Fixes #40.

@julen
Copy link
Contributor Author

julen commented Oct 26, 2016

I realize there are probably many ways to do this better, but I decided to give it a shot in order to get more acquainted with the codebase.

lib/Serge/Command/show.pm Outdated Show resolved Hide resolved
sub get_commands {
return {
show => {
handler => \&run,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we need to add need_config => 1, (see below)

@iafan
Copy link
Contributor

iafan commented Oct 27, 2016

On a separate note, I think the command needs a new name. 'show' (and I guess git show was an inspiration for the name here) could be used to show various props from the config file (to facilitate some scripting) serge show destination_languages <config.serge> would gather and report all destination languages from a config or multiple config files.

What we have here is an explanation of how config works. It's not an original config, but a transformed one. So I'm more inclined towards serge explain <config.serge> (inspired by SQL EXPLAIN) that would explain how the system would understand the config (which not only includes expanding inherited properties, but would also resolve environment variable-based macros).

@julen
Copy link
Contributor Author

julen commented Oct 31, 2016

Yes, git show was definitely the inspiration.

On a first read I said, ok, I'm fine making this explain, but after leaving this PR rest for a bit, part of me now thinks that showing props from the config file is what you described as expansion plus a filter on the output to only display what users specified on the CLI. So I'm not quite sure I agree this has to be two separate commands.

Getting back to the original inspiration, git show requires an <sha1> (or an alias to an sha1 such as HEAD, which is the default if no sha1 is provided), which will show the entire diff with respect to the previous changeset. The output can selectively be filtered, e.g. based on file path git show <sha1> -- path/or/file/to/display/. This is a pattern that other git commands support too, and it becomes very intuitive as users get involved with more commands.

Along those lines, I think for Serge it would be beneficial to keep the CLI surface as small as possible, and allow showing with a single command entire configuration files, as well as specific parts of them, because in the end they display an interpreted version of configuration files. For instance:

$ serge show foo.serge
<expanded contents of the config file>

$ serge show foo.serge destination_languages
<list of destination languages>

The option to filter on properties would require more thought (is the output displayed per-job? or does it merge properties across jobs? etc.). While it is very good we are already thinking about this use case, I'd say let's address its implementation separately.

When configurations use inheritance, it is handy to see the expanded
configuration to get a better feel of what is going to be run. While
Config::Neat already provides a command for this, having a `show`
command helps with the feature being discoverable.
my ($self) = @_;

my @config_files = $self->{parent}->get_config_files;
die "Multiple configuration files not allowed\n" unless $#config_files == 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if you would prefer to use $#config_files == 0 or rather scalar @config_files == 1. Please let me know if you have any preferences.

Copy link
Contributor

@prat0088 prat0088 Dec 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find scalar(@config_files) == 1 easier to read. $# is less common, requires extra Perl knowledge, and it looks like a check for a zero-length list if you're just skimming code.

You could further simplify the predicate to @config_files == 1.

@prat0088
Copy link
Contributor

Just checking in on the status of this PR. It looks really helpful. We're in the process of rolling out Serge and I was looking forward to using this feature to debug config files.

@iafan
Copy link
Contributor

iafan commented Dec 21, 2016

@prat0088 note that for debugging config files you can already use dump-nconf <yourconfig.serge>. This is a tool that comes with the underlying Config::Neat parser library.

This PR introduces the same functionality under serge show <yourconfig.serge> alias.

@codecov-io
Copy link

codecov-io commented Jul 24, 2019

Codecov Report

Merging #43 into master will decrease coverage by 0.04%.
The diff coverage is 30.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
- Coverage   54.51%   54.47%   -0.05%     
==========================================
  Files          93       94       +1     
  Lines        6954     6967      +13     
  Branches     1738     1739       +1     
==========================================
+ Hits         3791     3795       +4     
- Misses       2491     2500       +9     
  Partials      672      672
Impacted Files Coverage Δ
lib/Serge/Command/show.pm 30.76% <30.76%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8853c60...929efef. Read the comment docs.

@iafan iafan merged commit 1a2871d into serge-community:master Jul 25, 2019
@julen julen deleted the feature/show-command branch July 25, 2019 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to display expanded configurations
4 participants