Skip to content
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

Name a zipped extension file based on manifest #66

Merged
merged 1 commit into from
Feb 26, 2016

Conversation

kumar303
Copy link
Contributor

@kumar303
Copy link
Contributor Author

@rpl would you have time to review this? I'll send an email with some details.

@kumar303 kumar303 force-pushed the zip-name branch 10 times, most recently from 659cd70 to 0499c7b Compare February 25, 2016 16:19

return withTempDir(
(tmpDir) =>
adapter.buildMinimalExt(tmpDir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal: How about introducing co (at least in the test cases, because, in my own opinion, currently the implementation is clear enough even with the use of plain Promises) to get nicer async code?

as an example this test case could be turned into something like:

it('zips a package', () => withTempDir(
  (tmpDir) =>
     co(function*() {
       let zipFile = new ZipFile();

       let {extensionPath} = yield adapter.buildMinimalExt(tmpDir);

       assert.match(extensionPath, /minimal_extension-1\.0\.xpi$/);

       yield zipFile.open(extensionPath);

       let fileNames = [];

       yield zipFile.readEach((entry) => fileNames.push(entry.fileName));

       assert.deepEqual(fileNames, ['manifest.json']);
     })
));

and as a plus: this mostly matches to our current coding style for async tests in the Firefox source code (e.g. what is achieved in the test suites using add_task and Task.async

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omgyes! This is way more concise. I think we should convert the entire code base to this so I filed an issue for it: #72

@rpl
Copy link
Member

rpl commented Feb 26, 2016

@kumar303 Thanks a lot for asking me to review this, it was an helpful way to start to look into the internals of the newborn tool :-)

I found the code very clean and readable, then I haven't too much to add (just a couple of proposal and nits, that I hope can be useful)

👍

});


function writeManifest(destDir, manifestData) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, how about moving this helper in tests/helpers.js file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I didn't was because I'm not sure any other test will need this helper function. All other tests should be able to work directly with manifest objects.

kumar303 added a commit that referenced this pull request Feb 26, 2016
Name a zipped extension file based on manifest
@kumar303 kumar303 merged commit 5224d6d into mozilla:master Feb 26, 2016
@kumar303 kumar303 deleted the zip-name branch February 26, 2016 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants