613604 - "cfx init" should let you specify directory to initialize (and create it if needed)

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, enhancement, P2)

Description

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

"cfx init" works much like "git init" and "hg init", but "hg init" can initialize a directory other than the current working one if you specify it as an argument to the command.  And it'll create that directory if needed.  Both of which features make setting up a repository without an existing codebase simpler, since instead of:

  mkdir foo
  cd foo
  hg init

You can simply do:

  hg init foo

This use of the command is also safer, if used consistently, since it is less likely that you will enter the wrong name (and easier to recover from doing so) than be in the wrong directory.

Of course, you can still initialize an existing codebase by entering its directory and running "hg init" without arguments.

"cfx init" should work the same, initializing the directory specified as an argument to the command if one is specified and the current working directory otherwise.

Comment 1

[Julián Ceballos]

I proposed this idea at first time that we told about the init command. I like the idea and i'd like to implement it, if you accept.

Comment 2

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

Sounds good to me, go right ahead!

Comment 3

[Julián Ceballos]

Excuseme, i've update the code, but i have a doubt, will disabled the option to initialize an empty directory?

Comment 4

[Julián Ceballos]

Attachment: init with or without directory as argument

Sorry, the last question was a stupid question XD, i forgot the description of the process that said myk.
This is my update to the code in init to initialize an empty directory with "cfx init" or if the developer write "cfx init demo", create the demo directory and initialize.

Feedback?

Comment 5

[Brian Warner [:warner :bwarner]]

I like the overall idea: I think Julian's first version of this patch in bug
566814 used something closer to 'cfx init NEWDIR' than 'mkdir NEWDIR; cd
NEWDIR; cfx init', and I certainly find it safer to have 'cfx init' create a
directory when it runs.

I didn't realize it back then, but the comparison to git/hg isn't entirely
appropriate. Most of the time, when you run 'git init', you're taking an
existing source tree and starting to use version control on it (i.e. the
directory already has a bunch of files, which you very much want to
preserve). It's less common to use 'git init' in a brand new empty directory,
unless you've internalized the version-control mantra so well that you get
your VC environment set up before you start writing anything else.

So more likely parallels would be tools like "trac-admin init", or "svn-admin
init", which always create a new directory. Or some of my personal projects
(Tahoe-LAFS and Buildbot), which have some sort of "create new instance"
command that always take a NEWDIR argument.

So yeah, "cfx init NEWDIR" feels better to me than a zero-argument form that
modifies the current directory.

I'll look at the patch itself in a moment.

Comment 6

[Julián Ceballos]

I prefer only the directory name as an argument too.
If we give the easy way:
Cfx init addon_sample

Why keep the hard way?:
Mkdir addon_sample
Cd addon_sample
Cfx init

Regards!

Comment 7

[Brian Warner [:warner :bwarner]]

yeah, I'm willing to have it require the argument, and remove 'cfx init'. I'd ask for Atul's thoughts about that; I think he was the one who wanted the no-argument form in bug 566814.

Comment 8

[Brian Warner [:warner :bwarner]]

Comment on attachment 492038 [details] [diff] [review]
init with or without directory as argument

patch itself looks good, but:

* unit tests fail: they need to be updated
* output of "cfx init NEWDIR" should mention NEWDIR
** perhaps "lib directory created", etc, should say "NEWDIR/lib directory created"
** the instruction to do: try "cfx test" and then "cfx run" should maybe say:
   try "cd NEWDIR" then "cfx test" and "cfx run"
** or "Your sample add-on is now ready for testing" should say:
   "You new add-on (in NEWDIR) is now ready for testing"

I'm not sure the one-argument form should be called "init" . That verb makes sense for a zero-argument form, but a one-argument form wants a verb like "create" or "generate" or "new". I don't really know what a good word would be, though.

The test failure is the most important thing to fix.

Comment 9

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

(In reply to comment #8)
> I'm not sure the one-argument form should be called "init" . That verb makes
> sense for a zero-argument form, but a one-argument form wants a verb like
> "create" or "generate" or "new". I don't really know what a good word would be,
> though.

I think 'init' is the right word in this case, because it's familiar to folks who are used to hg, git, and even cvs, which all use "init" even when they create the directory on your behalf (although in the case of cvs you specify the directory to create in an environment variable rather than on the command line), with the notable exception of svnadmin, which strangely requires you to first create the directory yourself and then run the "create" command on it.

Comment 10

[Julián Ceballos]

(In reply to comment #8)
> Comment on attachment 492038 [details] [diff] [review]
> init with or without directory as argument
> 
> patch itself looks good, but:
> 
> * unit tests fail: they need to be updated
> * output of "cfx init NEWDIR" should mention NEWDIR
> ** perhaps "lib directory created", etc, should say "NEWDIR/lib directory
> created"
> ** the instruction to do: try "cfx test" and then "cfx run" should maybe say:
>    try "cd NEWDIR" then "cfx test" and "cfx run"
> ** or "Your sample add-on is now ready for testing" should say:
>    "You new add-on (in NEWDIR) is now ready for testing"
> 
> I'm not sure the one-argument form should be called "init" . That verb makes
> sense for a zero-argument form, but a one-argument form wants a verb like
> "create" or "generate" or "new". I don't really know what a good word would be,
> though.
> 
> The test failure is the most important thing to fix.

Hi Brian, yes i know that the test is important :S.

I have problems and i don't understand why, i've update the addon-sdk source code until:

commit 5792daa3ddf605935aaec3d4308dd38e03820487
Merge: 6252f40 e659cf6
Author: Atul Varma <[e-mail]>
Date:   Fri Nov 26 09:53:23 2010 -0500

    Bug 588119 - package.json should support add-on icon (r=warner, a=myk)

And i've seen that is changed the tabs import way to require('tabs') again.

Everything is ok, except this test:

exports.test_open_tab = function(test){
    require("tabs").open({
        url : "http://www.mozilla.org",
        onReady : function(tab){
            test.assertEqual(tab.location,"http://www.mozilla.org/");
            test.done();
        }
    });
    test.waitUntilDone(20000);
}

The error is:
...error: TEST FAILED: test-main.test_open_tab (exception)
error: An exception occurred.
Traceback (most recent call last):
  File "resource://demo-jetpack-core-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://demo-jetpack-core-lib/unit-test.js", line 223, in 
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://demo-jetpack-core-lib/unit-test.js", line 248, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://demo-jetpack-core-lib/unit-test.js", line 266, in start
    this.test.testFunction(this);
  File "resource://demo-jetpack-core-lib/unit-test-finder.js", line 57, in runTest
    test(runner);
  File "resource://demo-demo-tests/test-main.js", line 25, in 
    onReady : function(tab){
  File "resource://demo-addon-kit-lib/tabs.js", line 56, in open
    return browserWindows.activeWindow.tabs.open(options);
TypeError: browserWindows.activeWindow is null


If i use the same code in the lib/main.js everything is ok, obviously without the test. This is so rare. Do you know why it fails? or any idea?

Comment 11

[Julián Ceballos]

Attachment: init with or without directory as argument

This are the new files to generate the init template for addons, the rest of the patch, the initializer modifications are in the bug 613587.

Comment 13

[Brian Warner [:warner :bwarner]]

Comment on attachment 494303 [details] [diff] [review]
init with or without directory as argument

Could you make a patch which is self-contained? It looks like I need to apply both this patch and the one in bug 613587 to test either bug, which means I cannot review them separately.

I'd suggest making this patch add the init template and changing the initializer() function to use it. Then make a patch for bug 613587 which builds on top of this one (and let's mark that bug as depending upon this one).

Comment 14

[Brian Warner [:warner :bwarner]]

oh, also, when you're done, we should either remove the unused require()s from main.js, or we should add code to use them for something.

Comment 15

[Julián Ceballos]

Attachment: init new functionalities

This patch contains the init functionality to initialize a directory or create it if is neccesary. Too contains the funcionality to create the public/private keypair for the addon.

Comment 16

[Brian Warner [:warner :bwarner]]

Comment on attachment 499985 [details] [diff] [review]
init new functionalities

The approach looks good. Could you do the following?:

 - re-generate the patch against current trunk.. this patch doesn't apply cleanly
 - this patch also has a second copy of the init-template files, looks like a cut-and-paste error.. this needs to be removed to apply cleanly
 - in initializer(), use python's shutil.copytree() function to copy the whole contents of examples/init-template into the target directory: that should be a lot easier than manually mkdir/read/write each separate file, and will make future changes easier (just add a file to init-template/ , no need to also modify __init__.py)

thanks!
 -Brian

Comment 17

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

(automatic reprioritization of 1.0 bugs)

Comment 18

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

Brian: can you take this on, make the recommended changes, and then get final review from another reviewer?

Comment 19

[Brian Warner [:warner :bwarner]]

sure thing

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Link to pull request 462

On IRC, Warner said that changing cfx init to use the init-template dir can be done in a separate bug, and this one can just add the ability to specify a directory for cfx init to use.

So that's what this pull request does. It's basically what Julián's first version of his patch looked like, plus the fixed test in cuddlefish/tests/test_init.py.


When I do | cfx init myaddon | (when "myaddon" doesn't already exist), this is what it prints:
(C:\Users\KWierso\Documents\GitHub\addon-sdk) C:\Users\KWierso\Documents\packages\mypackage>cfx init myaddon
* myaddon package directory created
* lib directory created
* data directory created
* test directory created
* doc directory created
* README.md written
* package.json written
* test/test-main.js written
* lib/main.js written
* doc/main.md written

Your sample add-on is now ready.
Do "cfx test" to test it and "cfx run" to try it.  Have fun!




If I do | cfx init myaddon | and "myaddon" DOES exist, this is what prints:
(C:\Users\KWierso\Documents\GitHub\addon-sdk) C:\Users\KWierso\Documents\packages\mypackage>cfx init myaddon
* myaddon already exists, testing if directory is empty
* lib directory created
* data directory created
* test directory created
* doc directory created
* README.md written
* package.json written
* test/test-main.js written
* lib/main.js written
* doc/main.md written

Your sample add-on is now ready.
Do "cfx test" to test it and "cfx run" to try it.  Have fun!





If I do | cfx init myaddon | and "myaddon" exists and is not-empty:
(C:\Users\KWierso\Documents\GitHub\addon-sdk) C:\Users\KWierso\Documents\packages\mypackage>cfx init myaddon
* myaddon already exists, testing if directory is empty
This command must be run in an empty directory.





It doesn't add "myaddon\" to the beginning of each of the files and folders listed. Should it?

Comment 22

[Brian Warner [:warner :bwarner]]

Comment on attachment 631573 [details]
Link to pull request 462

I sent you a pull-request with a new test case. Add that to your branch and it looks good to go.

Comment 23

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/25891f4b5b04205027169179761b7518dee2cb74
Merge pull request #462 from KWierso/613604

Fix bug 613604 to allow cfx init to take a string argument and create the new addon in a folder using that string as a name.