-
Notifications
You must be signed in to change notification settings - Fork 52
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
Conversation
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. |
sub get_commands { | ||
return { | ||
show => { | ||
handler => \&run, |
There was a problem hiding this comment.
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)
On a separate note, I think the command needs a new name. 'show' (and I guess 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 |
Yes, On a first read I said, ok, I'm fine making this Getting back to the original inspiration, 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. |
255f8c7
to
191872b
Compare
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
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. |
@prat0088 note that for debugging config files you can already use This PR introduces the same functionality under |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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.