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

Support for generic templates #279

Merged
merged 8 commits into from Dec 21, 2015

Conversation

Projects
None yet
4 participants
@sd43
Contributor

sd43 commented Nov 9, 2015

  1. Moved template information out of p5-app-duckpan and into respective IA packages
  2. The templates are now defined in YAML files. See https://github.com/srvsh/zeroclickinfo-goodies/blob/3a4f248f61cdb343df37f0d84861129d91c9de7f/template/templates.yml for an example. The file also has some documentation about the structure of the YAML file.
  3. An instant answer can have multiple templates that can be chosen by the user during creation.
  4. A template has some required files and some optional files. Optional files are in groups and users must specify which of these groups of files they'd like to generate.
  5. App::DuckPAN::TemplateDefinitions is responsible for reading the YAML file and creating one App::DuckPAN::Template instance for each template.
  6. App::DuckPAN::Template is responsible for creating output files based on variables provided and a list of optional file groups. It also provides a list of output directories that App::DuckPAN::Restart watches for changes.

Usage:
duckpan new "Hello World" # creates an instant answer with the 'default' template
duckpan new --template cheatsheet "Hello World" # creates an instant answer with the 'cheatsheet' template

TODO:

  1. Tests
  2. An option to list all available templates
  3. Prompting the user to update the instant answer repository if a templates.yml file is not found

(this is part of work related to Issue #268)

@zekiel

This comment has been minimized.

Show comment
Hide comment
@zekiel

zekiel Nov 9, 2015

Member

this is an exciting PR. thanks @srvsh

Member

zekiel commented Nov 9, 2015

this is an exciting PR. thanks @srvsh

@sd43

This comment has been minimized.

Show comment
Hide comment
@sd43

sd43 Nov 13, 2015

Contributor

Thanks @zekiel !

I have now updated the PR with test cases and some other very minor modifications. I should be able to get to the pending tasks early next week.

Contributor

sd43 commented Nov 13, 2015

Thanks @zekiel !

I have now updated the PR with test cases and some other very minor modifications. I should be able to get to the pending tasks early next week.

@sd43 sd43 changed the title from Initial support for generic templates to Support for generic templates Nov 18, 2015

@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Nov 24, 2015

Contributor

Hey @srvsh, I just wanted to touch base on the template changes we discussed in the goodie PR. Do you want to make those changes as part of this?

Contributor

zachthompson commented Nov 24, 2015

Hey @srvsh, I just wanted to touch base on the template changes we discussed in the goodie PR. Do you want to make those changes as part of this?

@sd43

This comment has been minimized.

Show comment
Hide comment
@sd43

sd43 Nov 25, 2015

Contributor

Hi @zachthompson, yes, I'll send my updated code in this PR within a day. Sorry about the delay; I haven't been able to put in enough time for this yet.

Contributor

sd43 commented Nov 25, 2015

Hi @zachthompson, yes, I'll send my updated code in this PR within a day. Sorry about the delay; I haven't been able to put in enough time for this yet.

@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Nov 25, 2015

Contributor

@srvsh, np, just wanted to make sure you weren't waiting on me.

Contributor

zachthompson commented Nov 25, 2015

@srvsh, np, just wanted to make sure you weren't waiting on me.

@sd43

This comment has been minimized.

Show comment
Hide comment
@sd43

sd43 Nov 26, 2015

Contributor

@zachthompson I have implemented the new template format that you suggested with some minor modifications. The corresponding YAML file for goodies is in this pull request.

Following are some of the terminology I've used:

  1. The 'templates.yml' file is called 'the template definitions file
  2. A template consists of one input and one output file (eg. pm, css, etc)
  3. A template set is a collection of templates representing one instant answer sub-type (eg. for Goodies IA type we have default, cheatsheet and all sub-types). The word sub-type is used in messages to the user, but is synonymous to template set in the code. I thought that would be less ambiguous for the user since the word template is used for other things in the documentation.

As per your earlier comment, I have modified the format of the list of the available sub-types. I haven't included the initial prompt to skip viewing optional templates entirely with this set of changes though. (there's a TODO list at the end of this message)

Do let me know what you think!

Here's an example session using duckpan new with the changes:

srvsh$ duckpan new --list-subtypes
Available sub-types:
           all - Goodie with all backend and frontend files
    cheatsheet - Cheat Sheet Instant Answer
       default - Standard Goodie Instant Answer

srvsh$ duckpan new --subtype cheatsheet
Creating a new Cheat Sheet Instant Answer...
Please enter a name for your Instant Answer:  : readline
Created file: share/goodie/cheat_sheets/json/readline.json
Successfully created Goodie: Readline

srvsh$ duckpan new --subtype all
Creating a new Goodie with all backend and frontend files...
Please enter a name for your Instant Answer:  : weather forecast
Created file: lib/DDG/Goodie/WeatherForecast.pm
Created file: t/WeatherForecast.t
Created file: share/goodie/weather_forecast/weather_forecast.js
Created file: share/goodie/weather_forecast/weather_forecast.css
Successfully created Goodie: WeatherForecast

srvsh$ duckpan new
Creating a new Standard Goodie Instant Answer...
Please enter a name for your Instant Answer:  : bar code
Create Javascript file? [y/n] : y
Create CSS file? [y/n] : n
Created file: lib/DDG/Goodie/BarCode.pm
Created file: t/BarCode.t
Created file: share/goodie/bar_code/bar_code.js
Successfully created Goodie: BarCode

Pending tasks:

  1. Add a general prompt asking if the user wants to see the optional files
  2. Test cases
Contributor

sd43 commented Nov 26, 2015

@zachthompson I have implemented the new template format that you suggested with some minor modifications. The corresponding YAML file for goodies is in this pull request.

Following are some of the terminology I've used:

  1. The 'templates.yml' file is called 'the template definitions file
  2. A template consists of one input and one output file (eg. pm, css, etc)
  3. A template set is a collection of templates representing one instant answer sub-type (eg. for Goodies IA type we have default, cheatsheet and all sub-types). The word sub-type is used in messages to the user, but is synonymous to template set in the code. I thought that would be less ambiguous for the user since the word template is used for other things in the documentation.

As per your earlier comment, I have modified the format of the list of the available sub-types. I haven't included the initial prompt to skip viewing optional templates entirely with this set of changes though. (there's a TODO list at the end of this message)

Do let me know what you think!

Here's an example session using duckpan new with the changes:

srvsh$ duckpan new --list-subtypes
Available sub-types:
           all - Goodie with all backend and frontend files
    cheatsheet - Cheat Sheet Instant Answer
       default - Standard Goodie Instant Answer

srvsh$ duckpan new --subtype cheatsheet
Creating a new Cheat Sheet Instant Answer...
Please enter a name for your Instant Answer:  : readline
Created file: share/goodie/cheat_sheets/json/readline.json
Successfully created Goodie: Readline

srvsh$ duckpan new --subtype all
Creating a new Goodie with all backend and frontend files...
Please enter a name for your Instant Answer:  : weather forecast
Created file: lib/DDG/Goodie/WeatherForecast.pm
Created file: t/WeatherForecast.t
Created file: share/goodie/weather_forecast/weather_forecast.js
Created file: share/goodie/weather_forecast/weather_forecast.css
Successfully created Goodie: WeatherForecast

srvsh$ duckpan new
Creating a new Standard Goodie Instant Answer...
Please enter a name for your Instant Answer:  : bar code
Create Javascript file? [y/n] : y
Create CSS file? [y/n] : n
Created file: lib/DDG/Goodie/BarCode.pm
Created file: t/BarCode.t
Created file: share/goodie/bar_code/bar_code.js
Successfully created Goodie: BarCode

Pending tasks:

  1. Add a general prompt asking if the user wants to see the optional files
  2. Test cases
@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Nov 28, 2015

Contributor

Hi @srvsh The overall logic looks pretty good. I have some comments about the new terminology and @moollaza about the flow of duckpan new.

Let's stick with --template and --list-templates. I think --subtype could be confusing to users who don't know the code or config files for duckpan. They are our primary concern. Developers tweaking things can be expected to keep template in the YAML and --template at the command-line distinct.

For the output of files created, how about we condense it into:

Created files:
    lib/DDG/Goodie/BarCode.pm
    t/BarCode.t
    share/goodie/bar_code/bar_code.js

Are the two colons, e.g. : : readline, intentional?

Contributor

zachthompson commented Nov 28, 2015

Hi @srvsh The overall logic looks pretty good. I have some comments about the new terminology and @moollaza about the flow of duckpan new.

Let's stick with --template and --list-templates. I think --subtype could be confusing to users who don't know the code or config files for duckpan. They are our primary concern. Developers tweaking things can be expected to keep template in the YAML and --template at the command-line distinct.

For the output of files created, how about we condense it into:

Created files:
    lib/DDG/Goodie/BarCode.pm
    t/BarCode.t
    share/goodie/bar_code/bar_code.js

Are the two colons, e.g. : : readline, intentional?

@sd43

This comment has been minimized.

Show comment
Hide comment
@sd43

sd43 Nov 30, 2015

Contributor

@zachthompson thanks for your review and suggestions! I have changed the terminology to use 'template' instead of 'sub-type'. I have also modified the display of the created files. There are a couple of other minor changes renaming some identifiers in my code.

Are the two colons, e.g. : : readline, intentional?

This is not intentional, but the two colons were part of the prompt on my computer before my changes as well. When I run duckpan new, this is what I see:
Please enter a name for your Instant Answer: :
I haven't really investigated why it happens though.

I'll send out an update with the test cases today. Thanks!

Contributor

sd43 commented Nov 30, 2015

@zachthompson thanks for your review and suggestions! I have changed the terminology to use 'template' instead of 'sub-type'. I have also modified the display of the created files. There are a couple of other minor changes renaming some identifiers in my code.

Are the two colons, e.g. : : readline, intentional?

This is not intentional, but the two colons were part of the prompt on my computer before my changes as well. When I run duckpan new, this is what I see:
Please enter a name for your Instant Answer: :
I haven't really investigated why it happens though.

I'll send out an update with the test cases today. Thanks!

@@ -207,6 +207,12 @@ sub get_reply {
return $return;
}
sub ask_yn {

This comment has been minimized.

@zachthompson

zachthompson Nov 30, 2015

Contributor

Can we not just use get_reply?
Forget this comment!

@zachthompson

zachthompson Nov 30, 2015

Contributor

Can we not just use get_reply?
Forget this comment!

This comment has been minimized.

@sd43

sd43 Nov 30, 2015

Contributor

I feel that using Term::UI::ask_yn is useful since it does a couple of things automatically to save some code:

  1. It adds a '[y/n]' at the end of the prompt with the default option capitalized and
  2. Returns a boolean value after parsing the reply

But I understand your concern about defining a sub that is used only once, so I'll go with anything you're comfortable having in the code base.

@sd43

sd43 Nov 30, 2015

Contributor

I feel that using Term::UI::ask_yn is useful since it does a couple of things automatically to save some code:

  1. It adds a '[y/n]' at the end of the prompt with the default option capitalized and
  2. Returns a boolean value after parsing the reply

But I understand your concern about defining a sub that is used only once, so I'll go with anything you're comfortable having in the code base.

This comment has been minimized.

@zachthompson

zachthompson Nov 30, 2015

Contributor

I agree @srvsh. Ignore my question!

@zachthompson

zachthompson Nov 30, 2015

Contributor

I agree @srvsh. Ignore my question!

@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Nov 30, 2015

Contributor

Maybe the double colon is due to Term::UI? In the default example, if you add a colon to the prompt you get two of them:

my $term = Term::ReadLine->new("brand");

my $reply = $term->get_reply(
                prompt => "What is your favourite colour: ",
                choices => [qw|blue red green|],
                default => "blue",
);'

  1> blue
  2> red
  3> green

What is your favourite colour:  [1]:
Contributor

zachthompson commented Nov 30, 2015

Maybe the double colon is due to Term::UI? In the default example, if you add a colon to the prompt you get two of them:

my $term = Term::ReadLine->new("brand");

my $reply = $term->get_reply(
                prompt => "What is your favourite colour: ",
                choices => [qw|blue red green|],
                default => "blue",
);'

  1> blue
  2> red
  3> green

What is your favourite colour:  [1]:
@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Nov 30, 2015

Contributor

The colon is due to newer Term::UI treating the empty list it's passed differently. We run version 0.30 internally:

my $term = Term::ReadLine->new("brand");

my $reply = $term->get_reply(
                prompt => "What is your favourite colour: ", ()
);'
What is your favourite colour:

but on newer versions like 0.46:

my $term = Term::ReadLine->new("brand");

my $reply = $term->get_reply(
                prompt => "What is your favourite colour: ",
                ()                              
);'
What is your favourite colour:  : 

We could probably check %params and only add to the args if true.

Contributor

zachthompson commented Nov 30, 2015

The colon is due to newer Term::UI treating the empty list it's passed differently. We run version 0.30 internally:

my $term = Term::ReadLine->new("brand");

my $reply = $term->get_reply(
                prompt => "What is your favourite colour: ", ()
);'
What is your favourite colour:

but on newer versions like 0.46:

my $term = Term::ReadLine->new("brand");

my $reply = $term->get_reply(
                prompt => "What is your favourite colour: ",
                ()                              
);'
What is your favourite colour:  : 

We could probably check %params and only add to the args if true.

@sd43

This comment has been minimized.

Show comment
Hide comment
@sd43

sd43 Nov 30, 2015

Contributor

@zachthompson Ah, you're right. This commit in Term::UI introduced the new behaviour. From the looks of the it, I'm not sure if anything other than a version check on Term::UI can be help.

Perhaps in App::DuckPAN::get_reply() we can check if $Term::UI::VERSION >= 0.32 and remove the colon and/or spaces at the end. But this gets more messy if the prompt has a ? at the end instead of a colon, in which case the prompt will look like What is your username on https://duck.co/ ? :.

Contributor

sd43 commented Nov 30, 2015

@zachthompson Ah, you're right. This commit in Term::UI introduced the new behaviour. From the looks of the it, I'm not sure if anything other than a version check on Term::UI can be help.

Perhaps in App::DuckPAN::get_reply() we can check if $Term::UI::VERSION >= 0.32 and remove the colon and/or spaces at the end. But this gets more messy if the prompt has a ? at the end instead of a colon, in which case the prompt will look like What is your username on https://duck.co/ ? :.

@sd43

This comment has been minimized.

Show comment
Hide comment
@sd43

sd43 Dec 1, 2015

Contributor

@zachthompson I have made changes to show the new way of showing optional templates. Here's a sample session with the changes:

# List templates
srvsh$ duckpan new --list-templates
Available templates:
           all - Goodie with all backend and frontend files
    cheatsheet - Cheat Sheet Instant Answer
       default - Standard Goodie Instant Answer

# Default template with optional files
srvsh$ duckpan new vim
Creating a new Standard Goodie Instant Answer...
Would you like to configure optional templates? [y/N]: y

  1> Javascript
  2> CSS
  3> Javascript, CSS

Choose configuration [1]: 3
Created files:
    lib/DDG/Goodie/Vim.pm
    t/Vim.t
    share/goodie/vim/vim.js
    share/goodie/vim/vim.css
Successfully created Goodie: Vim

# Cheat Sheet - no optional files
srvsh$ duckpan new --template cheatsheet elinks
Creating a new Cheat Sheet Instant Answer...
Created files:
    share/goodie/cheat_sheets/json/elinks.json
Successfully created Goodie: Elinks

# On error
srvsh$ duckpan new --template cheatsheet w3m
Creating a new Cheat Sheet Instant Answer...
Created files:
    (none)
[FATAL]  Template output file 'share/goodie/cheat_sheets/json/w3m.json' already exists

Does this look alright?

Pending:

  1. Test cases (will start on this once the code changes are done)
  2. Showing a list of choices for templates instead of choosing a default template when no --template option is given. (I'm planning to do this in another PR)
Contributor

sd43 commented Dec 1, 2015

@zachthompson I have made changes to show the new way of showing optional templates. Here's a sample session with the changes:

# List templates
srvsh$ duckpan new --list-templates
Available templates:
           all - Goodie with all backend and frontend files
    cheatsheet - Cheat Sheet Instant Answer
       default - Standard Goodie Instant Answer

# Default template with optional files
srvsh$ duckpan new vim
Creating a new Standard Goodie Instant Answer...
Would you like to configure optional templates? [y/N]: y

  1> Javascript
  2> CSS
  3> Javascript, CSS

Choose configuration [1]: 3
Created files:
    lib/DDG/Goodie/Vim.pm
    t/Vim.t
    share/goodie/vim/vim.js
    share/goodie/vim/vim.css
Successfully created Goodie: Vim

# Cheat Sheet - no optional files
srvsh$ duckpan new --template cheatsheet elinks
Creating a new Cheat Sheet Instant Answer...
Created files:
    share/goodie/cheat_sheets/json/elinks.json
Successfully created Goodie: Elinks

# On error
srvsh$ duckpan new --template cheatsheet w3m
Creating a new Cheat Sheet Instant Answer...
Created files:
    (none)
[FATAL]  Template output file 'share/goodie/cheat_sheets/json/w3m.json' already exists

Does this look alright?

Pending:

  1. Test cases (will start on this once the code changes are done)
  2. Showing a list of choices for templates instead of choosing a default template when no --template option is given. (I'm planning to do this in another PR)
@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Dec 1, 2015

Contributor

@srvsh I created quite a few new instant answers and mostly looks good. One edge case:

zeroclickinfo-goodies {pr/1778}]$ dpng new --template
Creating a new Standard Goodie Instant Answer...
Would you like to configure optional templates? [y/N]: n
Created files:
    lib/DDG/Goodie/--template.pm
    t/--template.t
Successfully created Goodie: --template

What do you think about catching this and displaying a list of templates from which to choose? We could also just error but really what we want is for them to enter a template name.

Regarding your #2 above about duckpan new, let's leave it using the default template. I think we want a succinct way to do the default with no prompts.

Contributor

zachthompson commented Dec 1, 2015

@srvsh I created quite a few new instant answers and mostly looks good. One edge case:

zeroclickinfo-goodies {pr/1778}]$ dpng new --template
Creating a new Standard Goodie Instant Answer...
Would you like to configure optional templates? [y/N]: n
Created files:
    lib/DDG/Goodie/--template.pm
    t/--template.t
Successfully created Goodie: --template

What do you think about catching this and displaying a list of templates from which to choose? We could also just error but really what we want is for them to enter a template name.

Regarding your #2 above about duckpan new, let's leave it using the default template. I think we want a succinct way to do the default with no prompts.

@sd43

This comment has been minimized.

Show comment
Hide comment
@sd43

sd43 Dec 3, 2015

Contributor

Thanks for experimenting with the changes, @zachthompson! I investigated the issue with --template and no arguments, and it is in fact a more wider problem affecting all modules. I have submitted #286 with the fix. The issue was that the bin/duckpan script changes Getopt::Long's configuration which caused unwanted behaviour in this module.

The behaviour after that fix is that the duckpan new --template command chooses the default template and continues execution. It is not intuitive, but sadly that's how MooX::Options sets things up for options with default values. I have created an issue with that distribution: celogeek/MooX-Options#49

A workaround should be possible, but I think this case would be rare and since the options would be documented, it may not be too much of an issue. Do you think we can revisit this at a later date?

Contributor

sd43 commented Dec 3, 2015

Thanks for experimenting with the changes, @zachthompson! I investigated the issue with --template and no arguments, and it is in fact a more wider problem affecting all modules. I have submitted #286 with the fix. The issue was that the bin/duckpan script changes Getopt::Long's configuration which caused unwanted behaviour in this module.

The behaviour after that fix is that the duckpan new --template command chooses the default template and continues execution. It is not intuitive, but sadly that's how MooX::Options sets things up for options with default values. I have created an issue with that distribution: celogeek/MooX-Options#49

A workaround should be possible, but I think this case would be rare and since the options would be documented, it may not be too much of an issue. Do you think we can revisit this at a later date?

@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Dec 3, 2015

Member

@srvsh @zachthompson regarding the --template option, I believe we just need to add an option to the module like we've done in server.pm?

Nevermind, I see we've already tried that ;)

https://github.com/duckduckgo/p5-app-duckpan/blob/master/lib/App/DuckPAN/Cmd/Server.pm#L17

Member

moollaza commented Dec 3, 2015

@srvsh @zachthompson regarding the --template option, I believe we just need to add an option to the module like we've done in server.pm?

Nevermind, I see we've already tried that ;)

https://github.com/duckduckgo/p5-app-duckpan/blob/master/lib/App/DuckPAN/Cmd/Server.pm#L17

@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Dec 3, 2015

Contributor

@srvsh What I meant about the --template is just catching it when it's the sole argument around here, e.g.

if $self->list_templates || $args[0] eq '--template';

Then we at least error with an appropriate message if no template is supplied, rather than creating a --template.pm. In the future we could maybe enter the select a template dialog automatically.

Contributor

zachthompson commented Dec 3, 2015

@srvsh What I meant about the --template is just catching it when it's the sole argument around here, e.g.

if $self->list_templates || $args[0] eq '--template';

Then we at least error with an appropriate message if no template is supplied, rather than creating a --template.pm. In the future we could maybe enter the select a template dialog automatically.

@sd43

This comment has been minimized.

Show comment
Hide comment
@sd43

sd43 Dec 7, 2015

Contributor

Sounds good @zachthompson! I've made the change to display a list of templates when --template does not have an argument. Here's a sample run with the change:

srvsh$ duckpan new --template
[FATAL]  Please specify the template for your Instant Answer.
Available templates:
           all - Goodie with all backend and frontend files
    cheatsheet - Cheat Sheet Instant Answer
       default - Standard Goodie Instant Answer

Does that look alright?

Contributor

sd43 commented Dec 7, 2015

Sounds good @zachthompson! I've made the change to display a list of templates when --template does not have an argument. Here's a sample run with the change:

srvsh$ duckpan new --template
[FATAL]  Please specify the template for your Instant Answer.
Available templates:
           all - Goodie with all backend and frontend files
    cheatsheet - Cheat Sheet Instant Answer
       default - Standard Goodie Instant Answer

Does that look alright?

@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Dec 7, 2015

Contributor

👍

Contributor

zachthompson commented Dec 7, 2015

👍

@sd43

This comment has been minimized.

Show comment
Hide comment
@sd43

sd43 Dec 8, 2015

Contributor

@zachthompson I have made a change to the way the --template option with no parameters was handled and added the test cases.

There was an issue I found earlier which causes output from templates to have empty strings where the lia_id variable was used. I had made a temporary fix here which passes a lia_id with the same value as lia_name, since that seems to be the same as what the ID should be. Is that a correct assumption?

Contributor

sd43 commented Dec 8, 2015

@zachthompson I have made a change to the way the --template option with no parameters was handled and added the test cases.

There was an issue I found earlier which causes output from templates to have empty strings where the lia_id variable was used. I had made a temporary fix here which passes a lia_id with the same value as lia_name, since that seems to be the same as what the ID should be. Is that a correct assumption?

@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Dec 8, 2015

Contributor

@srvsh I just reviewed all of the places I could find where we are using lia_id/lia_name/ia_name. All of these really should be ia_id which is the lower-case version of the underscore path. The only name we use is the separated version.

@moollaza lmk if I missed anything but I think all of these should be ia_id, i.e. this should match the IA page.

Contributor

zachthompson commented Dec 8, 2015

@srvsh I just reviewed all of the places I could find where we are using lia_id/lia_name/ia_name. All of these really should be ia_id which is the lower-case version of the underscore path. The only name we use is the separated version.

@moollaza lmk if I missed anything but I think all of these should be ia_id, i.e. this should match the IA page.

@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Dec 9, 2015

Member

@srvsh we needed to make some minor updates to DuckPAN this week, can you pull in those changes so this PR is mergeable?I'll be testing this all out and hope to merge it asap.

BTW -- If you haven't responded to any of the automated messages about a free shirt, please do send us an email with your details if you'd like a free shirt! You've done some awesome work and we'd like to thank you 👍

Member

moollaza commented Dec 9, 2015

@srvsh we needed to make some minor updates to DuckPAN this week, can you pull in those changes so this PR is mergeable?I'll be testing this all out and hope to merge it asap.

BTW -- If you haven't responded to any of the automated messages about a free shirt, please do send us an email with your details if you'd like a free shirt! You've done some awesome work and we'd like to thank you 👍

sd43 added some commits Nov 26, 2015

Support for generic templates
Template information is now stored in respective IA packages for easier
extensibility. There's also support for optional files that users can
decide whether to generate.
Restart.pm: Use template info for monitoring
of directories which need the server to be restarted
New.pm: modify display of created files; wording
- Use the word 'template' instead of 'sub-type'
- Renamed get_reply_yes_no() to ask_yn() for consitency with Term::UI.
- Use tabs instead of spaces (introduced in an earlier commit) in New.pm for consitency
- Other minor changes to names and comments
Generic Templates: user interface changes
1. Ask user if they want to see optional templates
2. Show all possible combinations of optional templates in a menu

Also, made '_template_set' as an attribute of App::DuckPAN::New instead
of using a passing it around to methods.
@sd43

This comment has been minimized.

Show comment
Hide comment
@sd43

sd43 Dec 10, 2015

Contributor

@moollaza Done, I have rebased my changes on top of master so that there are no conflicts.

Thanks for the appreciation :) I did send a mail for a free T-shirt a few today :)

Regarding merging of this PR, I just wanted to let you know that it should be merged after duckduckgo/zeroclickinfo-spice#2295 and duckduckgo/zeroclickinfo-goodies#1778 have been merged to their respective repositories. The reason is that the code in this PR will error out in older Goodie and Spice repositories due to the missing templates.yml file.

Contributor

sd43 commented Dec 10, 2015

@moollaza Done, I have rebased my changes on top of master so that there are no conflicts.

Thanks for the appreciation :) I did send a mail for a free T-shirt a few today :)

Regarding merging of this PR, I just wanted to let you know that it should be merged after duckduckgo/zeroclickinfo-spice#2295 and duckduckgo/zeroclickinfo-goodies#1778 have been merged to their respective repositories. The reason is that the code in this PR will error out in older Goodie and Spice repositories due to the missing templates.yml file.

templates: Add subdir_support to TemplateSet.
This is a general addition, but it specifically allows us to error out
when the user provides a name containing path separators for Cheat
Sheets.
@sd43

This comment has been minimized.

Show comment
Hide comment
@sd43

sd43 Dec 10, 2015

Contributor

@moollaza I have incorporated your suggestions from here into this PR. There are a couple of changes:

  1. IA names entered by users are now validated by a basic regex to allow only characters from [/a-zA-Z0-9\s].
  2. A new subdir_support field has been added to TemplateSet.pm to specify which template sets support path separators in their names. The name entered by the user is checked for path separators for template sets that shouldn't have them.
  3. The 'success' message has been replaced by Success!

Here's a sample duckpan session with the changes:

srvsh$ duckpan new hello-world
Creating a new Standard Goodie Instant Answer...
[FATAL]  'hello-world' is not a valid name for an Instant Answer. Please run the program again and provide a valid name.

srvsh$ duckpan new duck/duckgo
Creating a new Standard Goodie Instant Answer...
Would you like to configure optional templates? [y/N]: y

  1> Javascript
  2> CSS
  3> Handlebars
  4> Javascript, CSS
  5> Javascript, Handlebars
  6> CSS, Handlebars
  7> Javascript, CSS, Handlebars

Choose configuration [1]: 7
Created files:
    lib/DDG/Goodie/Duck/Duckgo.pm
    t/Duck/Duckgo.t
    share/goodie/duck/duckgo/duck_duckgo.js
    share/goodie/duck/duckgo/duck_duckgo.css
    share/goodie/duck/duckgo/duck_duckgo.handlebars
Success!

srvsh$ duckpan new --template cheatsheet duck/duckhack
Creating a new Cheat Sheet Instant Answer...
[FATAL]  The name for this type of Instant Answer cannot contain path separators. Please run the program again and provide a valid name.

srvsh$ duckpan new --template cheatsheet duck duckhack
Creating a new Cheat Sheet Instant Answer...
Created files:
    share/goodie/cheat_sheets/json/duck_duckhack.json
Success!

Contributor

sd43 commented Dec 10, 2015

@moollaza I have incorporated your suggestions from here into this PR. There are a couple of changes:

  1. IA names entered by users are now validated by a basic regex to allow only characters from [/a-zA-Z0-9\s].
  2. A new subdir_support field has been added to TemplateSet.pm to specify which template sets support path separators in their names. The name entered by the user is checked for path separators for template sets that shouldn't have them.
  3. The 'success' message has been replaced by Success!

Here's a sample duckpan session with the changes:

srvsh$ duckpan new hello-world
Creating a new Standard Goodie Instant Answer...
[FATAL]  'hello-world' is not a valid name for an Instant Answer. Please run the program again and provide a valid name.

srvsh$ duckpan new duck/duckgo
Creating a new Standard Goodie Instant Answer...
Would you like to configure optional templates? [y/N]: y

  1> Javascript
  2> CSS
  3> Handlebars
  4> Javascript, CSS
  5> Javascript, Handlebars
  6> CSS, Handlebars
  7> Javascript, CSS, Handlebars

Choose configuration [1]: 7
Created files:
    lib/DDG/Goodie/Duck/Duckgo.pm
    t/Duck/Duckgo.t
    share/goodie/duck/duckgo/duck_duckgo.js
    share/goodie/duck/duckgo/duck_duckgo.css
    share/goodie/duck/duckgo/duck_duckgo.handlebars
Success!

srvsh$ duckpan new --template cheatsheet duck/duckhack
Creating a new Cheat Sheet Instant Answer...
[FATAL]  The name for this type of Instant Answer cannot contain path separators. Please run the program again and provide a valid name.

srvsh$ duckpan new --template cheatsheet duck duckhack
Creating a new Cheat Sheet Instant Answer...
Created files:
    share/goodie/cheat_sheets/json/duck_duckhack.json
Success!

@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Dec 21, 2015

Member
  1. IA names entered by users are now validated by a basic regex to allow only characters from [/a-zA-Z0-9\s].

We should still allow names to be entered as Perl Packages for non-cheatsheet IA: duckpan new DDG::Spice::My::Answers or duckpan new My::InstantAnswer

Member

moollaza commented Dec 21, 2015

  1. IA names entered by users are now validated by a basic regex to allow only characters from [/a-zA-Z0-9\s].

We should still allow names to be entered as Perl Packages for non-cheatsheet IA: duckpan new DDG::Spice::My::Answers or duckpan new My::InstantAnswer

@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Dec 21, 2015

Member

Otherwise, this LGTM 👍

Member

moollaza commented Dec 21, 2015

Otherwise, this LGTM 👍

##########################
# A 'template' for the user is equivalent to a 'template-set' for the program
option template => (

This comment has been minimized.

@moollaza

moollaza Dec 21, 2015

Member

a shortform, -t would be nice --- I'll try and compile of features that can be added later

@moollaza

moollaza Dec 21, 2015

Member

a shortform, -t would be nice --- I'll try and compile of features that can be added later

doc => 'template used to generate the instant answer skeleton (default: default)',
);
option list_templates => (

This comment has been minimized.

@moollaza

moollaza Dec 21, 2015

Member

likewise I think --list would be greatand/or-L`

@moollaza

moollaza Dec 21, 2015

Member

likewise I think --list would be greatand/or-L`

is => 'ro',
doc => 'list the available instant answer templates and exit',
);

This comment has been minimized.

@moollaza

moollaza Dec 21, 2015

Member

I'd still like a --cheatsheet and/or -c option to go straight to a cheat sheet

@moollaza

moollaza Dec 21, 2015

Member

I'd still like a --cheatsheet and/or -c option to go straight to a cheat sheet

@@ -0,0 +1,2 @@
package <:$package_name:>;
# <:$lia_name:>

This comment has been minimized.

@moollaza

moollaza Dec 21, 2015

Member

I guess we can remove this comment now?

@moollaza

moollaza Dec 21, 2015

Member

I guess we can remove this comment now?

##################################
my $package_name = 'MyInstantAnswer';
my $lia_name = 'my_instant_answer';

This comment has been minimized.

@moollaza

moollaza Dec 21, 2015

Member

@srvsh I think we need to remove reference to this now? Or change it to ia_id?

@moollaza

moollaza Dec 21, 2015

Member

@srvsh I think we need to remove reference to this now? Or change it to ia_id?

@moollaza moollaza assigned moollaza and unassigned zachthompson Dec 21, 2015

@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Dec 21, 2015

Member

This LGTM 👍

Member

moollaza commented Dec 21, 2015

This LGTM 👍

moollaza added a commit that referenced this pull request Dec 21, 2015

@moollaza moollaza merged commit bc8b252 into duckduckgo:master Dec 21, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Dec 22, 2015

Member

@srvsh as I've started using this new code I've found an error. Working with old pull requests that don't have the templates files creates problems. I suspect developers will see issues as well once DuckPAN is released.

The code falls into a loop warning that the templates can't be found -- we need to make it a non-fatal error when duckpan loads as usual, but a fatal error if you use the duckpan new function so that developers and i can continue with older branches.

Does that make sense?

Member

moollaza commented Dec 22, 2015

@srvsh as I've started using this new code I've found an error. Working with old pull requests that don't have the templates files creates problems. I suspect developers will see issues as well once DuckPAN is released.

The code falls into a loop warning that the templates can't be found -- we need to make it a non-fatal error when duckpan loads as usual, but a fatal error if you use the duckpan new function so that developers and i can continue with older branches.

Does that make sense?

@sd43

This comment has been minimized.

Show comment
Hide comment
@sd43

sd43 Dec 22, 2015

Contributor

@moollaza Thanks for your in-depth review! I'll work on your suggestions today.

The code falls into a loop warning that the templates can't be found -- we need to make it a non-fatal error when duckpan loads as usual, but a fatal error if you use the duckpan new function so that developers and i can continue with older branches.

Could you please give me some steps to reproduce your observation? If I run duckpan new in an older goodie/spice repository, I get the fatal error [FATAL] Template definitions file not found for Goodie Instant Answers. You may need to pull the latest version of this repository, after which the program exits. Should I do something else to reproduce the problem you're seeing?

Contributor

sd43 commented Dec 22, 2015

@moollaza Thanks for your in-depth review! I'll work on your suggestions today.

The code falls into a loop warning that the templates can't be found -- we need to make it a non-fatal error when duckpan loads as usual, but a fatal error if you use the duckpan new function so that developers and i can continue with older branches.

Could you please give me some steps to reproduce your observation? If I run duckpan new in an older goodie/spice repository, I get the fatal error [FATAL] Template definitions file not found for Goodie Instant Answers. You may need to pull the latest version of this repository, after which the program exits. Should I do something else to reproduce the problem you're seeing?

@sd43

This comment has been minimized.

Show comment
Hide comment
@sd43

sd43 Dec 22, 2015

Contributor

@moollaza I've opened PR #294 with a workaround for the problem you were facing. I think that should fix it.

Contributor

sd43 commented Dec 22, 2015

@moollaza I've opened PR #294 with a workaround for the problem you were facing. I think that should fix it.

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