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

duckpan new update #133

Merged
merged 10 commits into from Oct 16, 2014

Conversation

Projects
None yet
2 participants
@moollaza
Member

moollaza commented Oct 16, 2014

  • These changes were necessary for the new quick start tutorial to work.
  • Now the "name" can be a proper package name or filepath ("MyFirstSpice::Moollaza", or "MyFirstSpice/Moollaza") -- this is important so you can create a new Spice under a new or exists namespace, e.g. Transit::GoTransit
  • If the directories don't exist, they will be created
  • Big code cleanup and de-duplication
    • reordered the code, so that if the user is in Longtail or Fathead we stop executing before asking them for input
  • Also if you run it in the Fathead or Longtail repo it will gracefully die with an error message explaining they aren't supported

cc// @mwmiller please prioritize this if possible. If it's OKAY, would like to merge and then I'll make any necessary improvements after. This is needed for the new tutorial. Sorry about the rush!

Would love to write some tests for this, but I'm not sure about the best approach. I think I'd need to make a temporary directory and then populate it during the tests? Any thoughts on a testing approach would be helpful!

Show outdated Hide outdated lib/App/DuckPAN/Cmd/New.pm Outdated
@@ -216,7 +216,7 @@ sub camel_to_underscore {
my ($self, $name) = @_;
# Replace first capital by lowercase
# if followed my lowercase.
$name =~ s/^([A-Z])([a-z])/lc($1).$2/e;
$name =~ s/^([A-Z])([a-z])/lc($1).$2/ge;

This comment has been minimized.

@mwmiller

mwmiller Oct 16, 2014

Contributor

With this anchored at the front, I don't know how the g helps you.

@mwmiller

mwmiller Oct 16, 2014

Contributor

With this anchored at the front, I don't know how the g helps you.

$self->app->print_text("[ERROR] Sorry, DuckPAN does not support Longtails yet!");
exit -1;
} else {
$self->app->print_text("[ERROR] No lib/DDG/Goodie, lib/DDG/Spice, lib/DDG/Fathead or lib/DDG/Longtail found");

This comment has been minimized.

@mwmiller

mwmiller Oct 16, 2014

Contributor

I don't think this particular message should mention Fathead or Longtail. Gives a false impression as to what will happen if they do get into the correct directory.

I'd also tighten up the branching a bit. Make some kind of %dirs structure mapping the directory to the type (if necessary) and whether or not it is supported. Just to make life easier if there's a new IA-type (say, 'JoodieS') or we add Fathead support.

@mwmiller

mwmiller Oct 16, 2014

Contributor

I don't think this particular message should mention Fathead or Longtail. Gives a false impression as to what will happen if they do get into the correct directory.

I'd also tighten up the branching a bit. Make some kind of %dirs structure mapping the directory to the type (if necessary) and whether or not it is supported. Just to make life easier if there's a new IA-type (say, 'JoodieS') or we add Fathead support.

This comment has been minimized.

@moollaza

moollaza Oct 21, 2014

Member

Great suggestion! I've made a fix, will push soon...

@moollaza

moollaza Oct 21, 2014

Member

Great suggestion! I've made a fix, will push soon...

my $name = $self->app->phrase_to_camel($entered_name);
my ($path, $lc_path) = ("", "");

This comment has been minimized.

@mwmiller

mwmiller Oct 16, 2014

Contributor

I quite like this initialization idiom. So long as you are doing these two, why not:

my ($package_name, $separated_name, $path, $lc_path) = ($name, $name, "", "");
@mwmiller

mwmiller Oct 16, 2014

Contributor

I quite like this initialization idiom. So long as you are doing these two, why not:

my ($package_name, $separated_name, $path, $lc_path) = ($name, $name, "", "");

This comment has been minimized.

@moollaza

moollaza Oct 21, 2014

Member

👍 Fixed.

@moollaza

moollaza Oct 21, 2014

Member

👍 Fixed.

$separated_name =~ s/::/ /g;
if ($entered_name =~ m/::/) {
my @path_parts = split(/::/, $entered_name);

This comment has been minimized.

@mwmiller

mwmiller Oct 16, 2014

Contributor

I'd do the split first and then make sure I have more than one thing in there:

my @path_parts = split(/::/, $entered_name);
if (scalar @path_parts > 1) {
 ...;
} else {
# Error out on improperly entered name?
}

This might help resolve your potential leading/trailing :: problems. I think this happens early enough for that to be the case.

@mwmiller

mwmiller Oct 16, 2014

Contributor

I'd do the split first and then make sure I have more than one thing in there:

my @path_parts = split(/::/, $entered_name);
if (scalar @path_parts > 1) {
 ...;
} else {
# Error out on improperly entered name?
}

This might help resolve your potential leading/trailing :: problems. I think this happens early enough for that to be the case.

This comment has been minimized.

@moollaza

moollaza Oct 21, 2014

Member

👍 Fixed

@moollaza

moollaza Oct 21, 2014

Member

👍 Fixed

$path = join("/", @path_parts);
$lc_path = join("/", map { $self->app->camel_to_underscore($_) } @path_parts);
}
my $lc_name = $self->app->camel_to_underscore($name);
# %templates forms the spine data structure which is used
# as a guide to discovering content which is moved around
my %templates = (

This comment has been minimized.

@mwmiller

mwmiller Oct 16, 2014

Contributor

You could maybe move this up a bit to act as the %dir thing I mentioned earlier. The existence of files implies "supported".

@mwmiller

mwmiller Oct 16, 2014

Contributor

You could maybe move this up a bit to act as the %dir thing I mentioned earlier. The existence of files implies "supported".

This comment has been minimized.

@moollaza

moollaza Oct 21, 2014

Member

taking a slightly different approach. Making the dir check a function thats globally accessible...

@moollaza

moollaza Oct 21, 2014

Member

taking a slightly different approach. Making the dir check a function thats globally accessible...

my $filename = $io->filename;
my $filepath= $io->filepath;
$self->app->print_text("[ERROR] File already exists: \"$filename\" in $filepath");
exit -1;

This comment has been minimized.

@mwmiller

mwmiller Oct 16, 2014

Contributor

Nice clean up!

@mwmiller

mwmiller Oct 16, 2014

Contributor

Nice clean up!

ia_name => $name,
ia_package_name => $package_name,
ia_name_separated => $separated_name,
lia_name => $lc_path."_".$lc_name,

This comment has been minimized.

@mwmiller

mwmiller Oct 16, 2014

Contributor

I think $lc_path can still be '' here. Do you want to prepend an underscore in that case?

@mwmiller

mwmiller Oct 16, 2014

Contributor

I think $lc_path can still be '' here. Do you want to prepend an underscore in that case?

This comment has been minimized.

@moollaza

moollaza Oct 21, 2014

Member

Yes you're correct! Fixing right now...

@moollaza

moollaza Oct 21, 2014

Member

Yes you're correct! Fixing right now...

Goodie => {
dirs => [
"./lib/DDG/Goodie/$path",
"./t/$path",

This comment has been minimized.

@mwmiller

mwmiller Oct 16, 2014

Contributor

$path can be empty here which leads to output looking like ./t//Thingy.tand so on.

@mwmiller

mwmiller Oct 16, 2014

Contributor

$path can be empty here which leads to output looking like ./t//Thingy.tand so on.

This comment has been minimized.

@moollaza

moollaza Oct 16, 2014

Member

Whoops! I was so busy testing more complex cases, I forgot to double check the simplest case. I knew there was a reason I didn't do it this way first. Thanks for catching this!

@moollaza

moollaza Oct 16, 2014

Member

Whoops! I was so busy testing more complex cases, I forgot to double check the simplest case. I knew there was a reason I didn't do it this way first. Thanks for catching this!

This comment has been minimized.

@moollaza

moollaza Oct 21, 2014

Member

Should be fixed now...

@moollaza

moollaza Oct 21, 2014

Member

Should be fixed now...

}
$self->app->print_text("Successfully created $type: $package_name");

This comment has been minimized.

@mwmiller

mwmiller Oct 16, 2014

Contributor

You've done some work to clean up the package name (particularly as pertains to the leading/trailing :: stuff), but that doesn't show here.

In fact, I would just not even mention it. You've already told them where the files are.

@mwmiller

mwmiller Oct 16, 2014

Contributor

You've done some work to clean up the package name (particularly as pertains to the leading/trailing :: stuff), but that doesn't show here.

In fact, I would just not even mention it. You've already told them where the files are.

This comment has been minimized.

@moollaza

moollaza Oct 21, 2014

Member

My only counter argument is that when you give as input "foo bar baz", it'll show you that the Package FooBarBaz was created. Which I feel is a nice confirmation that everything was correctly interpreted.

I'll play around with it a little more and see...

@moollaza

moollaza Oct 21, 2014

Member

My only counter argument is that when you give as input "foo bar baz", it'll show you that the Package FooBarBaz was created. Which I feel is a nice confirmation that everything was correctly interpreted.

I'll play around with it a little more and see...

@mwmiller

This comment has been minimized.

Show comment
Hide comment
@mwmiller

mwmiller Oct 16, 2014

Contributor

@moollaza I've run this from the command line and I don't see any show-stoppers here, just minor issues with empty paths making strings "ugly". I'll go ahead and merge in service of helping your tutorial progress.

Contributor

mwmiller commented Oct 16, 2014

@moollaza I've run this from the command line and I don't see any show-stoppers here, just minor issues with empty paths making strings "ugly". I'll go ahead and merge in service of helping your tutorial progress.

mwmiller added a commit that referenced this pull request Oct 16, 2014

@mwmiller mwmiller merged commit 4775dc9 into master Oct 16, 2014

1 check passed

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

@mwmiller mwmiller deleted the zaahir/duckpan-new-update branch Oct 16, 2014

@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Oct 16, 2014

Member

@mwmiller thanks a lot for the merge and timely review! I'll definitely make a PR with some updates asap!

Member

moollaza commented Oct 16, 2014

@mwmiller thanks a lot for the merge and timely review! I'll definitely make a PR with some updates asap!

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