(PDK-370) Adds a 'pdk module generate' redirect to 'pdk new module'. #286

bmjen · 2017-09-05T21:43:27Z

This new CLI option is meant to mirror the 'puppet module generate' command for experienced module developers used to the old method of generating a module skeleton. This CLI option redirects to 'pdk new module' after an initial prompt confirming the user's intent.

bmjen · 2017-09-05T21:45:13Z

@puppetlabs/pdk and @rickmonro Please pull this and try out the new CLI subcommand for UX feedback.

I'll be adding unit and acceptance tests.

coveralls · 2017-09-05T22:11:41Z

Coverage decreased (-1.1%) to 92.76% when pulling 6035c27 on bmjen:pdk-370 into 32eee98 on puppetlabs:master.

DavidS · 2017-09-06T11:24:54Z

👍

The module subcommand can be marked as hidden so it doesn't show up in pdk help.
Pressing enter, instead of yes on the Did you mean... question throws a exception:

/home/david/gems/ruby/2.3.0/gems/tty-prompt-0.13.1/lib/tty/prompt/confirm_question.rb:125:in `block in conversion': undefined method `match' for true:TrueClass (NoMethodError)
Did you mean?  catch
	from /home/david/gems/ruby/2.3.0/gems/tty-prompt-0.13.1/lib/tty/prompt/converter_registry.rb:55:in `call'
	from /home/david/gems/ruby/2.3.0/gems/tty-prompt-0.13.1/lib/tty/prompt/converter_dsl.rb:18:in `convert'
	from /home/david/gems/ruby/2.3.0/gems/tty-prompt-0.13.1/lib/tty/prompt/question.rb:203:in `convert_result'
	from /home/david/gems/ruby/2.3.0/gems/tty-prompt-0.13.1/lib/tty/prompt/confirm_question.rb:78:in `render_question'
	from /home/david/gems/ruby/2.3.0/gems/tty-prompt-0.13.1/lib/tty/prompt/question.rb:121:in `render'
	from /home/david/gems/ruby/2.3.0/gems/tty-prompt-0.13.1/lib/tty/prompt/confirm_question.rb:65:in `call'
	from /home/david/gems/ruby/2.3.0/gems/tty-prompt-0.13.1/lib/tty/prompt.rb:314:in `yes?'
	from /home/david/git/pdk/lib/pdk/cli/util/command_redirector.rb:20:in `run'

DavidS · 2017-09-06T11:26:35Z

lib/pdk/cli/util/command_redirector.rb

+        def run
+          @prompt.print _('Did you mean \'')
+          @prompt.print pastel.bold(@command)
+          @prompt.puts _('\'?')


Translators will flag this as untranslatable.

It does work as @prompt.print _('Did you mean \'%{command}\'?') % { command: pastel.bold(@command) }.

DavidS · 2017-09-06T11:27:49Z

lib/pdk/cli/util/module_generate_helper.rb

+          opts[:target_dir] = target_dir.nil? ? module_name : target_dir
+
+          PDK.logger.info(_('Creating new module: %{modname}') % { modname: module_name })
+          PDK::Generate::Module.invoke(opts)


If PDK::Generate::Module.invoke is horrible enough to require a helper to be called, please fix the actual API, instead of layering on!

rickmonro · 2017-09-06T20:38:09Z

lib/pdk/cli/module/generate.rb

+  @module_generate_cmd = @module_cmd.define_command do
+    name 'generate'
+    usage _('generate [options] <module_name>')
+    summary _('This command is now \'pdk new module\'.')


Even a line of (info) would be helpful to assist the user here, so perhaps:

pdk (INFO): New modules are created using the ‘pdk new module’ command. Did you mean pdk new module? Y/n

Added the info line at line:23

coveralls · 2017-09-07T02:07:20Z

Coverage decreased (-1.2%) to 92.81% when pulling e5eba34 on bmjen:pdk-370 into ff57c56 on puppetlabs:master.

coveralls · 2017-09-07T02:08:23Z

Coverage decreased (-1.1%) to 92.866% when pulling d26929d on bmjen:pdk-370 into ff57c56 on puppetlabs:master.

coveralls · 2017-09-07T02:15:51Z

Coverage decreased (-1.06%) to 92.929% when pulling 86a5a85 on bmjen:pdk-370 into ff57c56 on puppetlabs:master.

coveralls · 2017-09-07T02:20:14Z

Coverage decreased (-1.06%) to 92.925% when pulling e83dd19 on bmjen:pdk-370 into ff57c56 on puppetlabs:master.

rodjek · 2017-09-07T04:56:49Z

lib/pdk/cli/module/generate.rb

+
+    PDK::CLI.template_url_option(self)
+
+    flag nil, 'skip-interview', _('When specified, skips interactive querying of metadata.')


Might be worth abstracting this flag out into its own method like template-url so that we only have 1 copy of the string if we want to change the wording in the future.

rodjek · 2017-09-07T05:09:32Z

lib/pdk/cli/module/generate.rb

+      end
+
+      PDK.logger.info(_('New modules are created using the ‘pdk new module’ command.'))
+      prompt = TTY::Prompt.new(help_color: :cyan)


This is a different colour to the help text in the rest of the prompts (probably a lot more readable than "bright black", not to mention a lot less painful to try to imagine). Should we adjust the help text in the interview to also use cyan for consistency?

@rodjek the interview also uses cyan as the help_color. The TTY::Prompt is just set up and passed into the interview from the generator.

https://github.com/puppetlabs/pdk/blob/master/lib/pdk/generators/module.rb#L217

coveralls · 2017-09-07T23:21:05Z

Coverage decreased (-55.8%) to 38.211% when pulling 88ae2a5 on bmjen:pdk-370 into ff57c56 on puppetlabs:master.

bmjen · 2017-09-08T00:20:08Z

spec/unit/cli/module/generate_spec.rb

+    end
+  end
+
+  # context 'with a no at the prompt' do


@rodjek need your help here. When I uncomment this context, rspec only runs the tests in this file and skips everything else.

@bmjen fixed that up, you needed to catch the SystemExit exception raised by PDK::CLI.run (because the code calls exit 1 which makes rspec exit)

coveralls · 2017-09-08T00:23:32Z

Coverage decreased (-0.1%) to 93.839% when pulling faee13b on bmjen:pdk-370 into ff57c56 on puppetlabs:master.

coveralls · 2017-09-08T02:15:18Z

Coverage decreased (-0.09%) to 93.898% when pulling cb83722 on bmjen:pdk-370 into ff57c56 on puppetlabs:master.

DavidS · 2017-09-08T10:06:11Z

lib/pdk/cli/module/generate.rb

+    PDK::CLI.template_url_option(self)
+    PDK::CLI.skip_interview(self)
+
+    flag nil, 'skip-interview', _('When specified, skips interactive querying of metadata.')


should make use of PDK::CLI.skip_interview(self)

lol. it does. Looks like I did an errant Ctrl-R in vim to readd this line. It's adding skip-interview twice. :(

DavidS · 2017-09-08T10:09:16Z

👍

coveralls · 2017-09-08T18:24:21Z

Coverage increased (+0.09%) to 93.993% when pulling 4473df7 on bmjen:pdk-370 into 8e5bbe5 on puppetlabs:master.

scotje

Left one nitpick naming comment but overall looks good!

scotje · 2017-09-08T21:54:59Z

lib/pdk/cli.rb

@@ -36,6 +37,10 @@ def self.template_url_option(dsl)
    dsl.option nil, 'template-url', _('Specifies the URL to the template to use when creating new modules or classes.'), argument: :required, default: PDK::Generate::Module.default_template_url
  end

+  def self.skip_interview(dsl)


nitpick: might call this skip_interview_option

(fixup) Remove extra skip-interview flag Adds accessor function for command redirect (PDK-370) Updates name of skip_interview helper

coveralls · 2017-09-08T22:24:53Z

Coverage increased (+0.09%) to 93.993% when pulling 375c007 on bmjen:pdk-370 into 8e5bbe5 on puppetlabs:master.

bmjen requested review from DavidS, scotje, rodjek, james-stocks and rickmonro September 5, 2017 21:43

bmjen changed the title ~~(PDK-370) Adds a 'pdk module generate' redirect to 'pdk new module'.~~ {wip}(PDK-370) Adds a 'pdk module generate' redirect to 'pdk new module'. Sep 5, 2017

DavidS reviewed Sep 6, 2017

View reviewed changes

rickmonro reviewed Sep 6, 2017

View reviewed changes

bmjen force-pushed the pdk-370 branch 4 times, most recently from 86a5a85 to e83dd19 Compare September 6, 2017 21:45

rodjek reviewed Sep 7, 2017

View reviewed changes

(maint) Updates Summary text and hides bundle subcommand.

27e13d9

bmjen force-pushed the pdk-370 branch from e83dd19 to 88ae2a5 Compare September 7, 2017 22:54

bmjen force-pushed the pdk-370 branch from 88ae2a5 to faee13b Compare September 8, 2017 00:16

bmjen commented Sep 8, 2017

View reviewed changes

rodjek force-pushed the pdk-370 branch from faee13b to cb83722 Compare September 8, 2017 02:05

DavidS reviewed Sep 8, 2017

View reviewed changes

DavidS added the feature label Sep 8, 2017

bmjen force-pushed the pdk-370 branch from cb83722 to 4473df7 Compare September 8, 2017 18:10

bmjen changed the title ~~{wip}(PDK-370) Adds a 'pdk module generate' redirect to 'pdk new module'.~~ (PDK-370) Adds a 'pdk module generate' redirect to 'pdk new module'. Sep 8, 2017

scotje approved these changes Sep 8, 2017

View reviewed changes

bmjen added 3 commits September 8, 2017 15:17

(PDK-370) Adds a 'pdk module generate' redirect to 'pdk new module'.

2458ddc

(fixup) Remove extra skip-interview flag Adds accessor function for command redirect (PDK-370) Updates name of skip_interview helper

(maint) Fixes conflicting shared_context warning

5f8a2b5

(PDK-370) Spec Tests for pdk module

375c007

bmjen force-pushed the pdk-370 branch from 4473df7 to 375c007 Compare September 8, 2017 22:17

bmjen merged commit 14e3bdb into puppetlabs:master Sep 8, 2017

bmjen deleted the pdk-370 branch September 8, 2017 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PDK-370) Adds a 'pdk module generate' redirect to 'pdk new module'. #286

(PDK-370) Adds a 'pdk module generate' redirect to 'pdk new module'. #286

bmjen commented Sep 5, 2017

bmjen commented Sep 5, 2017

coveralls commented Sep 5, 2017

DavidS commented Sep 6, 2017

DavidS Sep 6, 2017

DavidS Sep 6, 2017

rickmonro Sep 6, 2017 •

edited

Loading

bmjen Sep 6, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

rodjek Sep 7, 2017

rodjek Sep 7, 2017

bmjen Sep 7, 2017

coveralls commented Sep 7, 2017

bmjen Sep 8, 2017

rodjek Sep 8, 2017

coveralls commented Sep 8, 2017

coveralls commented Sep 8, 2017

DavidS Sep 8, 2017

bmjen Sep 8, 2017

DavidS commented Sep 8, 2017

coveralls commented Sep 8, 2017

scotje left a comment

scotje Sep 8, 2017

coveralls commented Sep 8, 2017


		PDK::CLI.template_url_option(self)

		flag nil, 'skip-interview', _('When specified, skips interactive querying of metadata.')

(PDK-370) Adds a 'pdk module generate' redirect to 'pdk new module'. #286

(PDK-370) Adds a 'pdk module generate' redirect to 'pdk new module'. #286

Conversation

bmjen commented Sep 5, 2017

bmjen commented Sep 5, 2017

coveralls commented Sep 5, 2017

DavidS commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickmonro Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 8, 2017

coveralls commented Sep 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidS commented Sep 8, 2017

coveralls commented Sep 8, 2017

scotje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 8, 2017

rickmonro Sep 6, 2017 •

edited

Loading