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

fix: Don't log info about the built extension when signing #577

Merged
merged 3 commits into from Oct 17, 2016

Conversation

Projects
None yet
4 participants
@saintsebastian
Copy link
Collaborator

commented Oct 15, 2016

Fix #181
Manually tested with example addon

@coveralls

This comment has been minimized.

Copy link

commented Oct 15, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 062d62a on saintsebastian:fix-181 into b433d43 on mozilla:master.

@kumar303
Copy link
Member

left a comment

I tested this out and it works great, thanks! I made a small request to fix some formatting.

Generally for patches like this we would need a test that ensures the run command is calling build() in a way that will execute the desired behavior. However, it's just a log statement in this case which is a bit harder to test so it's ok.

@@ -69,7 +69,8 @@ export default function sign(
}

let [buildResult, idFromSourceDir] = await Promise.all([
build({sourceDir, artifactsDir: tmpDir.path()}, {manifestData}),
build({sourceDir, artifactsDir: tmpDir.path()}, {manifestData,
showReadyMessage: false}),

This comment has been minimized.

Copy link
@kumar303

kumar303 Oct 17, 2016

Member

nitpicky request (sorry): when breaking a block into a new line, the break needs to be consistent. In this case the block is {manifestData, showReadyMessage: false}. You could make it break consistently in a couple different ways:

Here is an example of keeping all block elements on the same line:

build({sourceDir, artifactsDir: tmpDir.path()}, {
  manifestData, showReadyMessage: false,
}),

Here is an example of moving the entire block to a new line:

build({sourceDir, artifactsDir: tmpDir.path()}, 
      {manifestData, showReadyMessage: false}),

This type of consistency is hard to enforce with automatic lint rules so sorry about slowing you down to fix that. It's important to fix these kinds of things because otherwise other contributors may become distracted by the inconsistency when working on an unrelated feature.

This comment has been minimized.

Copy link
@saintsebastian

saintsebastian Oct 17, 2016

Author Collaborator

@kumar303 no problem, good to learn the something new

@coveralls

This comment has been minimized.

Copy link

commented Oct 17, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 10f8b7b on saintsebastian:fix-181 into b433d43 on mozilla:master.

@kumar303
Copy link
Member

left a comment

Sorry, one more nitpick. I'll try and see if I can add a lint rule to catch these :)

@@ -78,7 +79,8 @@ export type PackageCreatorFn =
(params: PackageCreatorParams) => Promise<ExtensionBuildResult>;

async function defaultPackageCreator(
{manifestData, sourceDir, fileFilter, artifactsDir}: PackageCreatorParams
{manifestData, sourceDir, fileFilter, artifactsDir,
showReadyMessage}: PackageCreatorParams

This comment has been minimized.

Copy link
@kumar303

kumar303 Oct 17, 2016

Member

oops, I completely missed this one. This is also formatted inconsistently because the block is only partially broken into the next line. An easy fix would be:

async function defaultPackageCreator({
  manifestData, sourceDir, fileFilter, artifactsDir, showReadyMessage
}: PackageCreatorParams): Promise<ExtensionBuildResult> {

This comment has been minimized.

Copy link
@saintsebastian

saintsebastian Oct 17, 2016

Author Collaborator

no worries, let me know if you see anything else

@coveralls

This comment has been minimized.

Copy link

commented Oct 17, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 7edc47f on saintsebastian:fix-181 into b433d43 on mozilla:master.

@kumar303
Copy link
Member

left a comment

Thanks!

@kumar303 kumar303 merged commit 441b1b6 into mozilla:master Oct 17, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.