566814 - cfx needs a stub package generator

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Dietrich Ayala (:dietrich)]

flightdeck will probably solve this need eventually, but it'd be nice to have on the cmd line also.

Comment 1

[Drew Willcoxon :adw]

Attachment: Julián's patch: adds a "makepack" command to cfx

This patch is from Julián from the following thread:

http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/75401908e3168f6c

Comment 2

[Julián Ceballos]

Hi, i'm Julián, My code actually is this:

def mkpkg(arguments=sys.argv[1:], env_root=os.path.abspath(os.path.dirname('__file__'))):
    if len(arguments) == 2:
        try:
            for d in ['','/lib','/data']:
                os.mkdir(env_root+'/'+arguments[1]+'/'+d)
        except:
            print >> sys.stderr, 'The Directory already exist.'
        open(env_root+'/'+arguments[1]+'/package.json','w').write('{\n\t"author":"",\n\t"description":"new addon"\n}')
        open(env_root+'/'+arguments[1]+'/lib/main.js','w').write('exports.main = function(){\n};')
    if len(arguments) > 2:
        print >> sys.stderr,'Too many arguments.'
    if len(arguments) < 2:
        print >> sys.stderr,'Please give me the name of your new package.'

Comment 3

[Julián Ceballos]

Oh, WTF 
I've wrote my code in this gists
¬¬ much better

http://gist.github.com/473366

Comment 4

[Julián Ceballos]

I've updated my code in the gist:
http://gist.github.com/473366

This code have a validation of modules and a complex template for an addon with an argument to add requires to the lib/main.js template.

Comment 5

[Drew Willcoxon :adw]

Julián, please attach your patch to this bug by clicking the "Add an attachment" link above.  If you need help making a patch, add another comment here.

Comment 6

[Julián Ceballos]

Yes i'd like to know how can i make a patch. please, i've read this https://developer.mozilla.org/en/Getting_your_patch_in_the_tree but i don't know if what i'll include is all the file that i've modified or just the peace of code that i've wrote.

Comment 7

[Julián Ceballos]

Attachment: Is a patch for the __init__.py to include a new option called mkpkg to automatize the creation of addons directories structure

To implement this patch in cuddlefish __init__.py just read at the beginning of file makepackage.py

To use the option just write:

cfx mkpkg addon

or to use the complex template:

cfx mkpkg addon with tabs,widget,context-menu

Comment 8

[Drew Willcoxon :adw]

The file you should attach is a patch file, also called a diff.  If you don't know what a patch or diff is, the page you linked to talks about it a little, and I encourage you to google it for more info.  You can look at the patch I attached to this bug to see what one looks like.  In order to make a patch, you need a copy of the Jetpack SDK source code.

Are you using Mercurial (also called hg) or git?

If neither, you should grab Mercurial and clone the Jetpack SDK repository.  These pages explain everything in order:

https://developer.mozilla.org/en/Installing_Mercurial
https://developer.mozilla.org/En/Developer_Guide/Source_Code/Mercurial
https://developer.mozilla.org/en/Creating_a_patch

The Jetpack SDK repository has the following address.  The second page above mentions cloning, and this is the address you need to clone:

http://hg.mozilla.org/labs/jetpack-sdk/

If you're using git, I think you can do a `git diff > patch`, but I don't know much else.

Comment 9

[Julián Ceballos]

Yes i've get the code from HG =) well, i'll try to do this diff, thanks a lot

Comment 10

[Julián Ceballos]

Oh yes of course, i've analized the information XD, it's easy make the patch is just get the diff of my clone XD. jeje!, i'm new programming in this kind of projects that are so big :S. Sorry. I'll do right now and i'll put into this bug.

Comment 11

[Julián Ceballos]

Attachment: Is a patch for the __init__.py to include a new option called mkpkg to automatize the creation of addon package with a template

To use this new function just type to use the simple template:

cfx mkpkg ADDON_NAME

or to use the complex template:

cfx mkpkg ADDON_NAME with widget,context-menu,tabs

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 460759 [details] [diff] [review]
Is a patch for the __init__.py to include a new option called mkpkg to automatize the creation of addon package with a template

Thanks Julián, your patch applies cleanly.  Let's ask Atul to review it.  A couple of things:  1) Please try to keep your coding style consistent with the rest of that file, including whitespace and line length.  2) You probably shouldn't hardcode a list of the current Jetpack modules.  If you do that, we'll have to update the list any time we add or remove a module.  It would be better to, for example, generate a list based on the files in the jetpack-core/lib directory.  Also, the user may want to initialize his add-on with another library besides jetpack-core, but your patch doesn't provide a way to do that.  Maybe that's not necessary, though.

Comment 13

[Julián Ceballos]

Attachment: Is a patch for the __init__.py to include a new option called mkpkg to automatize the creation of addons directories structure

I've modified my code in structure and the array with the modules listed instead by the code to read the jetpack-core directory. Too i've added code to let to the developer create the class that wants to include in the main.js and that is not in the jetpack-core modules.

Comment 14

[Julián Ceballos]

Attachment: Is a patch for the __init__.py to include a new option called mkpkg to automatize the creation of addons directories structure

I've replaced the content to write in the files of the addon, from the strings implicit, to a file called templates.py and importing the strings declared there, replacing the keywords with the content to write.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 461005 [details] [diff] [review]
Is a patch for the __init__.py to include a new option called mkpkg to automatize the creation of addons directories structure

Setting the reviewer to Atul.  Also requesting review from Brian, as he has done some thinking about a command like this.

Comment 16

[Atul Varma [:atul]]

Comment on attachment 461005 [details] [diff] [review]
Is a patch for the __init__.py to include a new option called mkpkg to automatize the creation of addons directories structure

Thanks for submitting this patch!

I tried it out and it seems to work, but I actually had to read the code to figure out how to use it--could you change python-lib/cuddlefish/__init__.py's "usage" variable to include documentation on mkpkg, so that it shows up as an option when someone runs 'cfx --help'?  It would also be swell if you could add more detailed docs to static-files/md/dev-guide/cfx-tool.md too.

I'm a bit concerned about the complex usage, in part because we don't have unit tests and ensuring the functionality always works could be an annoying burden in the future, especially if not many folks use it; maybe we could add the simple usage now and add the complex notation later if folks think it'd be really helpful?

Also, it's not that big a deal, but I'd appreciate if you could keep your line length to no more than 79 characters.

Finally, have you considered changing the name of this command to be 'cfx init' instead, and not requiring the directory parameter?  I'm just curious, as this would make cfx very familiar to those who know how to use git and hg, both of which have 'init' commands to create repositories in the current directory.

Comment 17

[Julián Ceballos]

(In reply to comment #16)
> Comment on attachment 461005 [details] [diff] [review]
> Is a patch for the __init__.py to include a new option called mkpkg to
> automatize the creation of addons directories structure
> 
> Thanks for submitting this patch!
> 
> I tried it out and it seems to work, but I actually had to read the code to
> figure out how to use it--could you change python-lib/cuddlefish/__init__.py's
> "usage" variable to include documentation on mkpkg, so that it shows up as an
> option when someone runs 'cfx --help'?  It would also be swell if you could add
> more detailed docs to static-files/md/dev-guide/cfx-tool.md too.
> 
> I'm a bit concerned about the complex usage, in part because we don't have unit
> tests and ensuring the functionality always works could be an annoying burden
> in the future, especially if not many folks use it; maybe we could add the
> simple usage now and add the complex notation later if folks think it'd be
> really helpful?
> 
> Also, it's not that big a deal, but I'd appreciate if you could keep your line
> length to no more than 79 characters.
> 
> Finally, have you considered changing the name of this command to be 'cfx init'
> instead, and not requiring the directory parameter?  I'm just curious, as this
> would make cfx very familiar to those who know how to use git and hg, both of
> which have 'init' commands to create repositories in the current directory.

Yes of course, i could add the lines of documentation to the usage variable in the python-lib/cuddlefish/__init__.py file and of course in the static-files/md/dev-guide/cfx-tool.md too.

I'll modified my code to the 79 characters per line.

And well, the change of name i'm not shure but we could try.

I'll upload the patch today at the night about everything that we've wrote.

It's okay?

Comment 18

[Atul Varma [:atul]]

Yep, sounds great, thanks!

Comment 19

[Julián Ceballos]

Attachment: Is a patch for the __init__.py to include a new option called mkpkg to automatize the creation of addons directories structure

I've add a patch with a basic implementation of the `mkpkg` option now renamed to `init` for cfx command in jetpack.
The new implementations is:
$ mkdir myaddon
$ cd myaddon
$ cfx init

Comment 20

[Julián Ceballos]

The last attachment was added considering a basic implementation, to test if this is helpful.

Comment 21

[Julián Ceballos]

I have a doubt, how can we analize the impressions of developers about this kind of patches?
this patches are added to a beta version of SDK or something like that?

Comment 22

[Atul Varma [:atul]]

Awesome! I like it.

I'm not sure how to analyze the impressions of developers without releasing it as part of an actual SDK release. One idea I had a while ago was to actually support extensions to cfx the same way hg does, and then make most/all current cfx commands into extensions, but that would require some nontrivial refactoring.

Since this change is an enhancement and doesn't break existing functionality, though, I think we can just add it into the next version of the SDK and see how folks like it.

Comment 23

[Julián Ceballos]

That would be awesome.
I don't know maybe ask feedback, about what new functions they would like to get in the SDK?.
To make so much fast their addons creation.

Comment 24

[Atul Varma [:atul]]

Julian, this patch doesn't seem to contain the templates.py file... or am I not seeing it?

Also, I am willing to review this patch if warner is busy atm.

Comment 25

[Julián Ceballos]

Attachment: init command for cfx, initialize default structure in jetpack addons

Patch updated, here is templates.py and __init__.py patches.

Comment 26

[Julián Ceballos]

(In reply to comment #24)
> Julian, this patch doesn't seem to contain the templates.py file... or am I not
> seeing it?
> 
> Also, I am willing to review this patch if warner is busy atm.

Here it is, sorry =), so in the reviewer i change from warnes to you?

Comment 27

[Brian Warner [:warner :bwarner]]

Comment on attachment 467291 [details]
init command for cfx, initialize default structure in jetpack addons

Looks good overall. Some fixes to make:

* __init__.py line 313:
   path = os.path.abspath(os.path.dirname('__file__'))
 - this should just be "path = os.getcwd()". The use of '__file__' is
   suggestive of the C language's magic __FILE__ macro, which tells you the
   pathname of the enclosing .c file, but Python doesn't work that way at all
   and using '__file__' is just confusing. (if you wanted to locate the
   jetpack-sdk directory to e.g. copy some non-embedded template files
   contained therein, you'd either start with sys.argv[0], or something like
   pkg_resources.resource_filename(), but since templates.py contains the
   templates as embedded strings, neither is necessary).
* line 314 should be: addon = os.path.basename(path)
 - if we only ran on unix/OS-X, then addon=path.split('/')[:-1] would work
 - but on DOS, that path.split won't see the backslashes. os.path.basename
   will do the right thing on each platform.

* templates.py:
 - this file doesn't need a #! prefix: it will never be executed as a script
 - all files should end with a newline
 - please end triple-quoted strings with a newline too: the PACKAGE_JSON
   string will result in a packages.json that will look confusing when dumped
   to a terminal. End the file with "version":"0.1"\n}\n'''\n
 - style thing: don't use the same delimiter (quote or apostrophe) for both
   triple-quoted strings and for single-quoted strings inside them. i.e.
   please use:
     MAIN_JS_REQUIRES ='''var %(module)s = require("%(module)s");
     '''
   instead of:
     MAIN_JS_REQUIRES ='''var %(module)s = require('%(module)s');
     '''
   This helps syntax-highlighting tools which otherwise get lost in all the
   confusing quotes
 - it looks like MAIN_JS_REQUIRES is unused
 - how about using "name" or "package_name" as the substitution variable of
   "packName" ? I tend to prefer easy-to-read names over abbreviations until
   the length of the abbreviation gets in the way.

* sample add-on content
 - it'd be nice if the sample add-on created by "cfx init" did something more
   visible.. maybe use the Widget API to add a button to the UI.
 - in the sample exports.main() function: should it have "options" and
   "callbacks" arguments? I didn't think those were really exposed to user
   code.
 - test-main.js: don't all test case functions need to begin with "test"?
 - in general, I'd like the output of 'cfx init' to both do something
   interesting and show something interesting when tests are run, since it
   will serve as the starting point for many users. They should be able to
   immediately do 'cfx test' and 'cfx run' and see obvious results for both.

* the help text needs to mention that "cfx init" creates a sample add-on *in
  the current directory*. A number of other "init" tools (git init, svnadmin
  create) will create a subdirectory instead of modifying the current
  directory, so users may be surprised to have a bunch of files clutter up
  the wrong directory.

* never use bare "except" clauses: they will hide other errors and display
  confusing messages (what if PACKAGE_JSON were changed to use %(name)s
  instead of %(packName)s, but __init__.py weren't updated to match: that
  would be displayed unhelpfully as "The project is already defined"). For
  __init__.py, I'd use something like:
    if os.path.exists("README.md") or os.path.exists("package.json"):
        print >>sys.stderr, "The project is already defined"
  instead of the try/except block. If you do choose to go with the
  try/except, use "except EnvironmentError" to limit the scope of the catch.
  Also notice that you're only expecting exceptions in the os.mkdir() call
  (which will throw OSError when called with a pre-existing directory), since
  all of the calls to open()/write() would succeed (and append a second copy
  of the template to the existing file).

* this should probably have a unit test. To test the error cases,
  initializer() should accept a stderr=sys.stderr argument instead of
  hard-coding sys.stderr, and the test case can provide a StringIO object.
  For some ideas, look in python-lib/cuddlefish/tests/test_preflight.py and
  search for StringIO and make_basedir().

Comment 28

[Julián Ceballos]

(In reply to comment #27)
> Comment on attachment 467291 [details]
> init command for cfx, initialize default structure in jetpack addons
> 
> Looks good overall. Some fixes to make:
> 
> * __init__.py line 313:
>    path = os.path.abspath(os.path.dirname('__file__'))
>  - this should just be "path = os.getcwd()". The use of '__file__' is
>    suggestive of the C language's magic __FILE__ macro, which tells you the
>    pathname of the enclosing .c file, but Python doesn't work that way at all
>    and using '__file__' is just confusing. (if you wanted to locate the
>    jetpack-sdk directory to e.g. copy some non-embedded template files
>    contained therein, you'd either start with sys.argv[0], or something like
>    pkg_resources.resource_filename(), but since templates.py contains the
>    templates as embedded strings, neither is necessary).
> * line 314 should be: addon = os.path.basename(path)
>  - if we only ran on unix/OS-X, then addon=path.split('/')[:-1] would work
>  - but on DOS, that path.split won't see the backslashes. os.path.basename
>    will do the right thing on each platform.
> 
> * templates.py:
>  - this file doesn't need a #! prefix: it will never be executed as a script
>  - all files should end with a newline
>  - please end triple-quoted strings with a newline too: the PACKAGE_JSON
>    string will result in a packages.json that will look confusing when dumped
>    to a terminal. End the file with "version":"0.1"\n}\n'''\n
>  - style thing: don't use the same delimiter (quote or apostrophe) for both
>    triple-quoted strings and for single-quoted strings inside them. i.e.
>    please use:
>      MAIN_JS_REQUIRES ='''var %(module)s = require("%(module)s");
>      '''
>    instead of:
>      MAIN_JS_REQUIRES ='''var %(module)s = require('%(module)s');
>      '''
>    This helps syntax-highlighting tools which otherwise get lost in all the
>    confusing quotes
>  - it looks like MAIN_JS_REQUIRES is unused
>  - how about using "name" or "package_name" as the substitution variable of
>    "packName" ? I tend to prefer easy-to-read names over abbreviations until
>    the length of the abbreviation gets in the way.
> 
> * sample add-on content
>  - it'd be nice if the sample add-on created by "cfx init" did something more
>    visible.. maybe use the Widget API to add a button to the UI.
>  - in the sample exports.main() function: should it have "options" and
>    "callbacks" arguments? I didn't think those were really exposed to user
>    code.
>  - test-main.js: don't all test case functions need to begin with "test"?
>  - in general, I'd like the output of 'cfx init' to both do something
>    interesting and show something interesting when tests are run, since it
>    will serve as the starting point for many users. They should be able to
>    immediately do 'cfx test' and 'cfx run' and see obvious results for both.
> 
> * the help text needs to mention that "cfx init" creates a sample add-on *in
>   the current directory*. A number of other "init" tools (git init, svnadmin
>   create) will create a subdirectory instead of modifying the current
>   directory, so users may be surprised to have a bunch of files clutter up
>   the wrong directory.
> 
> * never use bare "except" clauses: they will hide other errors and display
>   confusing messages (what if PACKAGE_JSON were changed to use %(name)s
>   instead of %(packName)s, but __init__.py weren't updated to match: that
>   would be displayed unhelpfully as "The project is already defined"). For
>   __init__.py, I'd use something like:
>     if os.path.exists("README.md") or os.path.exists("package.json"):
>         print >>sys.stderr, "The project is already defined"
>   instead of the try/except block. If you do choose to go with the
>   try/except, use "except EnvironmentError" to limit the scope of the catch.
>   Also notice that you're only expecting exceptions in the os.mkdir() call
>   (which will throw OSError when called with a pre-existing directory), since
>   all of the calls to open()/write() would succeed (and append a second copy
>   of the template to the existing file).
> 
> * this should probably have a unit test. To test the error cases,
>   initializer() should accept a stderr=sys.stderr argument instead of
>   hard-coding sys.stderr, and the test case can provide a StringIO object.
>   For some ideas, look in python-lib/cuddlefish/tests/test_preflight.py and
>   search for StringIO and make_basedir().

Ok, this weekend i could review the suggestions by Warner, about the init code, and talk about it, in the next meeting at 7th August, 2010.

Comment 29

[Julián Ceballos]

(In reply to comment #27)
> Comment on attachment 467291 [details]
> init command for cfx, initialize default structure in jetpack addons
> 
> * never use bare "except" clauses: they will hide other errors and display
>   confusing messages (what if PACKAGE_JSON were changed to use %(name)s
>   instead of %(packName)s, but __init__.py weren't updated to match: that
>   would be displayed unhelpfully as "The project is already defined"). For
>   __init__.py, I'd use something like:
>     if os.path.exists("README.md") or os.path.exists("package.json"):
>         print >>sys.stderr, "The project is already defined"
>   instead of the try/except block. If you do choose to go with the
>   try/except, use "except EnvironmentError" to limit the scope of the catch.
>   Also notice that you're only expecting exceptions in the os.mkdir() call
>   (which will throw OSError when called with a pre-existing directory), since
>   all of the calls to open()/write() would succeed (and append a second copy
>   of the template to the existing file).

Hi, what do you think about change this:
>     if os.path.exists("README.md") or os.path.exists("package.json"):
>         print >>sys.stderr, "The project is already defined"

instead:

if len(os.listdir(os.getcwd()))>0:
  print >> sys.stderr, "The project is already defined"

Comment 30

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #28)
> Ok, this weekend i could review the suggestions by Warner, about the init code,
> and talk about it, in the next meeting at 7th August, 2010.

Note that Brian is away for the next week, so he won't be at the meeting tomorrow.  Thus, probably the best next step would be for you to attach an updated patch based on Brian's review comments along with any questions you have (just like you did in comment 29!) to this bug report.  Then, when Brian returns, he can review the updated patch and answer any questions you posted with it.

Comment 31

[Julián Ceballos]

Attachment: init command for cfx, create a sample addon in current directory

This is the new cfx init patch for jetpack sdk, this is a function to create a sample addon in the current directory, by default this addon use widget, tabs, request and self libraries in the main.js and test-main.js

The one thing that i've not implemented is this recommendation by Warner:

>* this should probably have a unit test. To test the error cases,
>  initializer() should accept a stderr=sys.stderr argument instead of
>  hard-coding sys.stderr, and the test case can provide a StringIO object.
>  For some ideas, look in python-lib/cuddlefish/tests/test_preflight.py and
>  search for StringIO and make_basedir().

Because i'd like to read the review of this new patch version.

Comment 32

[Julián Ceballos]

Attachment: init command for cfx, create a sample addon in current directory

This is the new cfx init patch for jetpack sdk, this is a function to create a
sample addon in the current directory, by default this addon use widget, tabs,
request and self libraries in the main.js and test-main.js

The one thing that i've not implemented is this recommendation by Warner:

>* this should probably have a unit test. To test the error cases,
>  initializer() should accept a stderr=sys.stderr argument instead of
>  hard-coding sys.stderr, and the test case can provide a StringIO object.
>  For some ideas, look in python-lib/cuddlefish/tests/test_preflight.py and
>  search for StringIO and make_basedir().

Because i'd like to read the review of this new patch version.

Last comment was wrong ¬¬ jiji!.

Comment 33

[Brian Warner [:warner :bwarner]]

Comment on attachment 472712 [details] [diff] [review]
init command for cfx, create a sample addon in current directory

It's looking a lot better! Just a few more small things to change:

indentation: please use 4-space indents (with real spaces, not tabs), to
match the rest of the file (your "initializer()" function, at least, uses
tabs, and Python will rightly complain about mixing tabs and spaces in the
same file). In general, please use spaces instead of tabs.

The cfx-tool.md docs should probably say "Just create a new directory, change
into it, and run `cfx init`.", to emphasize the fact that you must run this
from a new directory.

The "if len(os.listdir(path)) > 0" check sounds good. I'd spell if "if
os.listdir(path)", but either way is pretty easy to read. I'd change the
error message to "The project %s is already defined: this tool must be run in
an empty directory.", so that users will have a way to figure out why we
think it's already defined.

You can remove some unnecessary indentation in initializer() by 'return'ing
from the error cases. The result is usually easier to follow, and doesn't
wander off the right-hand side of the screen. For example, instead of this:

 if foo:
    complain(error)
 else:
    do stuff
    if bar:
      complain(other error)
    else:
      do other stuff

you can do:

 if foo:
    complain(error)
    return
 do stuff
 if bar:
    complain (other error)
    return
 do other stuff


Minor spelling nit: "> package.json wrote" should be "> package.json
written", and " README.md writed" should also be " README.md written". I'd
change the last message to 'Your sample add-on is now ready for testing: try
"cfx test" and then "cfx run". Have fun!'.

Also, it'd still be nice to have a unit test for this, which should probably
go in python-lib/cuddlefish/tests/test_init.py, and should:

 create an empty directory
 run 'cfx init' in it
 check to see that at least one of the expected files was created
 check the contents of that file
 run 'cfx init' again in that directory, confirm that it throws an error

thanks!
 -Brian

Comment 34

[Julián Ceballos]

I've update the patch, i'm working in the unittest for init ;). Maybe tomorrow i'll upload the patch for review =).

Comment 35

[Julián Ceballos]

Attachment: init command for cfx, create a sample addon in current directory

Update of cfx init patch.

command unittest, doc, addon_sample unittest, __init__.py code

Everything is done, i wait review =).

Best regards

Comment 36

[Brian Warner [:warner :bwarner]]

Comment on attachment 478544 [details] [diff] [review]
init command for cfx, create a sample addon in current directory

The code is looking really good! Just a few more changes:

 - typo in the cfx-tool.md docs: I'd say "This command will create the
   following file structure:" instead of "This commando..". Also I'd mention
   that this command "creates a skeleton add-on, as a starting point for your
   own add-on development", since it's not currently obvious what 'cfx init'
   is good for.

 - the unit test suite fails on my machine (when running 'cfx testcfx')

  - unit tests should never call os.chdir() (unless they're really careful to
    change it back aftwards, even if an exception occurs). This will mess up
    later unit tests which expect to be run from a specific directory

  - you cannot depend upon unit tests being run in any particular order: they
    should each run independently, and work even if just a single test is
    run.

  - I try to avoid using things like os.system("cfx init") (since it depends
    upon PATH being set right, and spawns off a new process). Atul may feel
    differently, and I'd follow his lead, but I'd suggest just importing and
    invoking initializer() directly

  - unit tests shouldn't write anything to stdout/stderr, because it gets
    interleaved with the sucess/failure outputs of all the other tests. I
    usually handle this by passing stdout/stderr objects into the function
    under test, letting them default to sys.stdout/sys.stderr . See
    test_preflight.py and cuddlefish/preflight.py for some examples. This
    will also let you test that running 'cfx init' with arguments or in a
    pre-existing directory does what it's supposed to do (return an error and
    complain usefully).

  - you can use self.fail("why") instead of self.assertFalse(1). Likewise
    just use 'pass' instead of 'self.assertTrue(1)'. Actually, if you want to
    assert that calling a function causes a certain exception to be raised,
    you should do self.assertRaises(EXCEPTIONNAME, function, args), which has
    the added benefit that the test will fail if the wrong exception is
    raised: a bareword-except (using just "except:" instead of "except
    EXCEPTIONNAME") is almost always a bad idea, as it will hide unexpected
    exceptions.

 - test_init.py needs to end with a newline


So I'd suggest a unit test that looks more like this:

====

import os
import unittest
import shutil
from StringIO import StringIO

from cuddlefish.__init__ import initializer
from cuddlefish.templates import MAIN_JS, TEST_MAIN_JS, PACKAGE_JSON


class TestInit(unittest.TestCase):

    def run_in_subdir(self, dirname, f, *args, **kwargs):
        top = os.path.abspath(os.getcwd())
        basedir = os.path.abspath(os.path.join("_test_tmp", self.id(), dirname))
        if os.path.isdir(basedir):
            assert basedir.startswith(top) # safety
            shutil.rmtree(basedir)
        os.makedirs(basedir)
        try:
            os.chdir(basedir)
            return f(basedir, *args, **kwargs)
        finally:
            # always leave CWD where we found it
            os.chdir(top)

    def do_test_initialize(self, basedir):
        out,err = StringIO(), StringIO()
        rc = initializer(None, ["init"], out, err)
        out = out.getvalue(); err = err.getvalue()
        self.assertEqual(rc, 0)
        self.assertTrue("> lib directory created" in out)
        self.assertTrue("> data directory created" in out)
        self.assertTrue("Your sample add-on is now ready for testing" in out)
        self.assertEqual(err, "")
        self.assertTrue(os.listdir(basedir) > 0)
        main_js = os.path.join(basedir,'lib','main.js')
        package_json = os.path.join(basedir,'package.json')
        self.assertTrue(os.path.exists(main_js))
        self.assertTrue(os.path.exists(package_json))
        self.assertEqual(open(main_js,'r').read(),MAIN_JS)
        self.assertEqual(open(package_json,'r').read(),
                         PACKAGE_JSON%{'name':'tmp_addon_sample'})
        test_main_js = os.path.join(basedir,'tests','test-main.js')
        self.assertEqual(open(test_main_js,'r').read(),TEST_MAIN_JS)

        # now check that 'cfx init' in a pre-existing directory fails
        out,err = StringIO(), StringIO()
        rc = initializer(None, ["init"], out, err)
        out = out.getvalue(); err = err.getvalue()
        self.failIfEqual(rc, 0)
        self.assertTrue("This tool must be run in an empty directory" in err)

    def test_initialize(self):
        self.run_in_subdir("tmp_addon_sample", self.do_test_initialize)

    def do_test_badargs(self, basedir):
        # check that running it with spurious arguments will fail
        out,err = StringIO(), StringIO()
        rc = initializer(None, ["init", "ignored-dirname"], out, err)
        out = out.getvalue(); err = err.getvalue()
        self.failIfEqual(rc, 0)
        self.assertTrue("Too many arguments" in err, err)
    def test_badargs(self):
        self.run_in_subdir("tmp_addon_sample", self.do_test_badargs)

if __name__ == '__main__':
    unittest.main()

=====

and some corresponding changes to initializer():

=====

-def initializer(env_root, args):
+def initializer(env_root, args, out=sys.stdout, err=sys.stderr):
     from templates import MAIN_JS, PACKAGE_JSON, README_DOC, MAIN_JS_DOC, TEST_MAIN_JS
     path = os.getcwd()
     addon = os.path.basename(path)
     #if more than one argument
     if len(args) > 1:
-        print >> sys.stderr, 'Too many arguments.'
-        return
+        print >>err, 'Too many arguments.'
+        return 1
     #if current dir isn't empty
     if len(os.listdir(path)) > 0:
-        print >> sys.stderr, 'This tool must be run in an empty directory.'
-        return
+        print >>err, 'This tool must be run in an empty directory.'
+        return 1
     for d in ['lib','data','tests','docs']:
         os.mkdir(os.path.join(path,d))
-        print >> sys.stdout, '>', d, 'directory created'
+        print >>out, '>', d, 'directory created'
     open('README.md','w').write(README_DOC % {'name':addon})
-    print >> sys.stdout, '> README.md written'
+    print >>out, '> README.md written'
     open('package.json','w').write(PACKAGE_JSON % {'name':addon})
-    print >> sys.stdout, '> package.json written'
+    print >>out, '> package.json written'
     open(os.path.join(path,'tests','test-main.js'),'w').write(TEST_MAIN_JS)
-    print >> sys.stdout, '> tests/test-main.js written'
+    print >>out, '> tests/test-main.js written'
     open(os.path.join(path,'lib','main.js'),'w').write(MAIN_JS)
-    print >> sys.stdout, '> lib/main.js written'
+    print >>out, '> lib/main.js written'
     open(os.path.join(path,'docs','main.md'),'w').write(MAIN_JS_DOC)
-    print >> sys.stdout, '> docs/main.md written'
-    print >> sys.stdout, '''Your sample add-on is now ready for testing:
+    print >>out, '> docs/main.md written'
+    print >>out, '''Your sample add-on is now ready for testing:
     try "cfx test" and then "cfx run". Have fun!"'''
+    return 0

=====

with those changes, this will be ready for landing! thanks!
 -Brian

Comment 37

[Brian Warner [:warner :bwarner]]

oh, also, when I run the generated unit tests (mkdir q; cd q; cfx init; cfx test), I get 4-of-4 passing, but there's an awful lot of "JavaScript Warning" noise, I think maybe because the widget that is used to display www.mozilla.org is not fully-featured enough to handle all the crazy CSS and jQuery that we put there? I think it will be less scary for developers if the sample unit test suite passes cleanly without warnings.

Comment 38

[Julián Ceballos]

(In reply to comment #36)
> Comment on attachment 478544 [details] [diff] [review]
> init command for cfx, create a sample addon in current directory
> 
> The code is looking really good! Just a few more changes:
> 
>  - typo in the cfx-tool.md docs: I'd say "This command will create the
>    following file structure:" instead of "This commando..". Also I'd mention
>    that this command "creates a skeleton add-on, as a starting point for your
>    own add-on development", since it's not currently obvious what 'cfx init'
>    is good for.
> 
>  - the unit test suite fails on my machine (when running 'cfx testcfx')
> 
>   - unit tests should never call os.chdir() (unless they're really careful to
>     change it back aftwards, even if an exception occurs). This will mess up
>     later unit tests which expect to be run from a specific directory
> 
>   - you cannot depend upon unit tests being run in any particular order: they
>     should each run independently, and work even if just a single test is
>     run.
> 
>   - I try to avoid using things like os.system("cfx init") (since it depends
>     upon PATH being set right, and spawns off a new process). Atul may feel
>     differently, and I'd follow his lead, but I'd suggest just importing and
>     invoking initializer() directly
> 
>   - unit tests shouldn't write anything to stdout/stderr, because it gets
>     interleaved with the sucess/failure outputs of all the other tests. I
>     usually handle this by passing stdout/stderr objects into the function
>     under test, letting them default to sys.stdout/sys.stderr . See
>     test_preflight.py and cuddlefish/preflight.py for some examples. This
>     will also let you test that running 'cfx init' with arguments or in a
>     pre-existing directory does what it's supposed to do (return an error and
>     complain usefully).
> 
>   - you can use self.fail("why") instead of self.assertFalse(1). Likewise
>     just use 'pass' instead of 'self.assertTrue(1)'. Actually, if you want to
>     assert that calling a function causes a certain exception to be raised,
>     you should do self.assertRaises(EXCEPTIONNAME, function, args), which has
>     the added benefit that the test will fail if the wrong exception is
>     raised: a bareword-except (using just "except:" instead of "except
>     EXCEPTIONNAME") is almost always a bad idea, as it will hide unexpected
>     exceptions.
> 
>  - test_init.py needs to end with a newline
> 
> 
> So I'd suggest a unit test that looks more like this:
> 
> ====
> 
> import os
> import unittest
> import shutil
> from StringIO import StringIO
> 
> from cuddlefish.__init__ import initializer
> from cuddlefish.templates import MAIN_JS, TEST_MAIN_JS, PACKAGE_JSON
> 
> 
> class TestInit(unittest.TestCase):
> 
>     def run_in_subdir(self, dirname, f, *args, **kwargs):
>         top = os.path.abspath(os.getcwd())
>         basedir = os.path.abspath(os.path.join("_test_tmp", self.id(),
> dirname))
>         if os.path.isdir(basedir):
>             assert basedir.startswith(top) # safety
>             shutil.rmtree(basedir)
>         os.makedirs(basedir)
>         try:
>             os.chdir(basedir)
>             return f(basedir, *args, **kwargs)
>         finally:
>             # always leave CWD where we found it
>             os.chdir(top)
> 
>     def do_test_initialize(self, basedir):
>         out,err = StringIO(), StringIO()
>         rc = initializer(None, ["init"], out, err)
>         out = out.getvalue(); err = err.getvalue()
>         self.assertEqual(rc, 0)
>         self.assertTrue("> lib directory created" in out)
>         self.assertTrue("> data directory created" in out)
>         self.assertTrue("Your sample add-on is now ready for testing" in out)
>         self.assertEqual(err, "")
>         self.assertTrue(os.listdir(basedir) > 0)
>         main_js = os.path.join(basedir,'lib','main.js')
>         package_json = os.path.join(basedir,'package.json')
>         self.assertTrue(os.path.exists(main_js))
>         self.assertTrue(os.path.exists(package_json))
>         self.assertEqual(open(main_js,'r').read(),MAIN_JS)
>         self.assertEqual(open(package_json,'r').read(),
>                          PACKAGE_JSON%{'name':'tmp_addon_sample'})
>         test_main_js = os.path.join(basedir,'tests','test-main.js')
>         self.assertEqual(open(test_main_js,'r').read(),TEST_MAIN_JS)
> 
>         # now check that 'cfx init' in a pre-existing directory fails
>         out,err = StringIO(), StringIO()
>         rc = initializer(None, ["init"], out, err)
>         out = out.getvalue(); err = err.getvalue()
>         self.failIfEqual(rc, 0)
>         self.assertTrue("This tool must be run in an empty directory" in err)
> 
>     def test_initialize(self):
>         self.run_in_subdir("tmp_addon_sample", self.do_test_initialize)
> 
>     def do_test_badargs(self, basedir):
>         # check that running it with spurious arguments will fail
>         out,err = StringIO(), StringIO()
>         rc = initializer(None, ["init", "ignored-dirname"], out, err)
>         out = out.getvalue(); err = err.getvalue()
>         self.failIfEqual(rc, 0)
>         self.assertTrue("Too many arguments" in err, err)
>     def test_badargs(self):
>         self.run_in_subdir("tmp_addon_sample", self.do_test_badargs)
> 
> if __name__ == '__main__':
>     unittest.main()
> 
> =====
> 
> and some corresponding changes to initializer():
> 
> =====
> 
> -def initializer(env_root, args):
> +def initializer(env_root, args, out=sys.stdout, err=sys.stderr):
>      from templates import MAIN_JS, PACKAGE_JSON, README_DOC, MAIN_JS_DOC,
> TEST_MAIN_JS
>      path = os.getcwd()
>      addon = os.path.basename(path)
>      #if more than one argument
>      if len(args) > 1:
> -        print >> sys.stderr, 'Too many arguments.'
> -        return
> +        print >>err, 'Too many arguments.'
> +        return 1
>      #if current dir isn't empty
>      if len(os.listdir(path)) > 0:
> -        print >> sys.stderr, 'This tool must be run in an empty directory.'
> -        return
> +        print >>err, 'This tool must be run in an empty directory.'
> +        return 1
>      for d in ['lib','data','tests','docs']:
>          os.mkdir(os.path.join(path,d))
> -        print >> sys.stdout, '>', d, 'directory created'
> +        print >>out, '>', d, 'directory created'
>      open('README.md','w').write(README_DOC % {'name':addon})
> -    print >> sys.stdout, '> README.md written'
> +    print >>out, '> README.md written'
>      open('package.json','w').write(PACKAGE_JSON % {'name':addon})
> -    print >> sys.stdout, '> package.json written'
> +    print >>out, '> package.json written'
>      open(os.path.join(path,'tests','test-main.js'),'w').write(TEST_MAIN_JS)
> -    print >> sys.stdout, '> tests/test-main.js written'
> +    print >>out, '> tests/test-main.js written'
>      open(os.path.join(path,'lib','main.js'),'w').write(MAIN_JS)
> -    print >> sys.stdout, '> lib/main.js written'
> +    print >>out, '> lib/main.js written'
>      open(os.path.join(path,'docs','main.md'),'w').write(MAIN_JS_DOC)
> -    print >> sys.stdout, '> docs/main.md written'
> -    print >> sys.stdout, '''Your sample add-on is now ready for testing:
> +    print >>out, '> docs/main.md written'
> +    print >>out, '''Your sample add-on is now ready for testing:
>      try "cfx test" and then "cfx run". Have fun!"'''
> +    return 0
> 
> =====
> 
> with those changes, this will be ready for landing! thanks!
>  -Brian

- What do you think about this for the cfx-tools.md? "This command will create an skeleton addon, as a starting point for your own add-on development with next files structure:"

- I've add your suggestions to the code of unittest and initializer ,thanks a lot.

Best regards.

Comment 39

[Julián Ceballos]

Attachment: init command for cfx, create a sample addon in current directory

I wait feedback :). Thanks.

Comment 40

[Julián Ceballos]

Attachment: init command for cfx, create a sample addon in current directory

I've found little bug in my patch, here i attach a new version, ready to review :). Thanks!.

Comment 41

[Brian Warner [:warner :bwarner]]

Comment on attachment 485036 [details] [diff] [review]
init command for cfx, create a sample addon in current directory

looks great! I'll land it in a moment.

Comment 42

[Brian Warner [:warner :bwarner]]

Landed in git 5249b86f069e18290c177e8958c316d32a0a46b1, aka http://github.com/mozilla/addon-sdk/commit/5249b86f069e18290c177e8958c316d32a0a46b1 

I made a few cosmetic changes on the way (minor python style things), and I took the liberty of fixing the skeleton's unit test to match the current way that the Request API works (the onComplete callback gets a 'response' argument, rather than using 'this.response'), since the test was failing otherwise.

thanks!

Comment 43

[Julián Ceballos]

Great!, thank you too.

Comment 44

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".