Automatically ignore --out path #255

develar · 2016-02-05T12:49:59Z

This pull request supersedes #155. And is required for #251.

Tests and doc added.

malept · 2016-02-05T15:16:31Z

I would prefer that since you're building on #155, that you rebase + squash that branch so that the original author is still attributed to their changes, and then an additional commit is for you adding the tests and docs requested.

develar · 2016-02-05T16:26:07Z

Sorry for noise, I created pull request too early (realized, that my test is not correct). I will add comment when it will be ready. Yeah, writing tests is not easy.

develar · 2016-02-05T17:23:38Z

Strange. Tests passed locally, but not on Travis. And in your build (https://travis-ci.org/maxogden/electron-packager/jobs/107157456) I see the same error. I will start build again in a few hours.

develar · 2016-02-05T17:25:19Z

so that the original author is still attributed to their changes

Fixed, I set author to pyro2927 <[email protected]>.

develar · 2016-02-07T08:23:33Z

I don't see what's wrong in my code. And Travis build passed. (I see that another unrelated build also blinking).

Maybe, it will be better to test such functionality only for current platform (not for all platforms) to speedup tests, but I used standard util.testAllPlatforms.

malept · 2016-02-07T16:18:53Z

Could you add one more test for the new ignore functionality when you don't specify an out? (That's what I meant in the other PR for a "current working directory case".

develar · 2016-02-08T06:30:48Z

Could you add one more test for the new ignore functionality when you don't specify an out

It is required to fix #64 I don't like it, but as a tool we cannot teach users, ok. Current solution is not complete, thanks for pointing.

malept · 2016-02-08T07:57:48Z

common.js

+  var outIgnores = []
+  if (normalizedOut === null || normalizedOut === process.cwd()) {
+    ['darwin', 'linux', 'win32'].forEach(function (platform) {
+      ['ia32', 'x64'].forEach(function (arch) {


I would move supportedArchs and supportedPlatforms in index.js to common.js and use those instead. Otherwise, that will make PRs like #107 and #223 more complex than they need to be.

develar · 2016-02-09T06:41:57Z

Tests failed due to critical error in all tests —

you pass combination to test and then call Object.create.
Created opts has combination as a prototype.
createInferTest calls delete combination.version directly on original combination instance and... yes, version deleted from prototype and all tests now could be broken.

As part of this pull request I will fix it (we must pass copied combination object to test to avoid manual Object.create and to avoid such problems in the future).

develar · 2016-02-09T07:27:27Z

I would move supportedArchs and supportedPlatforms in index.js to common.js

Sorry, tried to minimise, but it is stupid, yes. Fixed.

malept · 2016-02-09T07:33:40Z

index.js

+var supportedPlatforms = common.platforms.reduce(function (o, v) {
+  o[v] = './' + (v === 'darwin' ? 'mac' : v)
+  return o
+}, {})


In my opinion, this makes it a bit more difficult to understand what's going on.

I also prefer to use for of instead of magic function style. But I see forEach in existing code.

Do you want to convert it to

var supportedPlatforms = {} for (var platform in common.platforms) { supportedPlatforms[platform] = './' + (platform === 'darwin' ? 'mac' : platform) }

?

At the very least, please don't use single-letter variables here. Please also keep in mind that there is a PR to add the Mac App Store (MAS) platform (#223) which currently also uses the mac.js file.

Variables renamed. I don't want to pollute file scope, but cannot use let, so, still use reduce instead of for in (for of es6).

Mac App Store (MAS) platform can be added as before — directly to supportedPlatforms (or inside reduce function — in any case we check is equal to darwin?).

I think with your current code, the MAS change will look like this:

result[platform] = './' + (['darwin', 'mas'].indexOf(platform) !== -1 ? 'mac' : platform)

It's not great.

Or

var supportedPlatforms = common.platforms.reduce(function (result, platform) { result[platform] = './' + (platform === 'darwin' ? 'mac' : platform) return result }, { mas: './mac' })

Seems like you may as well do either option for darwin as well and get rid of the ternary operator. (Though I'm not convinced that the second option actually works.)

It seems, you don't like ternary operator :)

Ok, my code saves only 2 lines (linux and windows). Maybe we can rename mac.js to darwin.js to consolidate, but I don't think that it is an acceptable solution.

So, I reverted this block to previous.

I'm OK with ternary operators when all parts of the statement are simple.

Travis build passed, please review.

develar · 2016-02-18T09:25:42Z

Should I fix something or you just haven't yet time to review it?

malept · 2016-02-18T15:53:33Z

Sorry, I've been holding off reviewing/merging other PRs because #223 is bigger than the others. Hopefully in the next couple of days.

malept · 2016-02-18T17:12:33Z

Yep, I figured that you would need to rebase after I merged the PR I mentioned earlier.

develar · 2016-02-22T07:32:30Z

@malept I missed your quick reply, sorry. I have rebased.

…g directory and we exclude previous artifacts directories)

…ect — version property was deleted from prototype

Automatically ignore --out path Closes #155.

develar force-pushed the ignore-out-path branch from a4f7f75 to 2f3b4a0 Compare February 5, 2016 13:20

malept mentioned this pull request Feb 5, 2016

Ability to set tmpdir to false to disable using temp directory #251

Merged

develar force-pushed the ignore-out-path branch 5 times, most recently from 861f1bd to 274cfaf Compare February 5, 2016 16:20

develar force-pushed the ignore-out-path branch from 274cfaf to 55e7bb3 Compare February 5, 2016 16:52

develar force-pushed the ignore-out-path branch 2 times, most recently from 2249101 to 23e41ff Compare February 7, 2016 07:50

develar force-pushed the ignore-out-path branch 2 times, most recently from 2fe28c3 to ba65340 Compare February 8, 2016 07:50

malept reviewed Feb 8, 2016
View reviewed changes

malept reviewed Feb 9, 2016
View reviewed changes

develar force-pushed the ignore-out-path branch 3 times, most recently from 5a9cf3b to 1b1d8d2 Compare February 9, 2016 19:54

develar mentioned this pull request Feb 10, 2016

provide way to create "installers" (.dmg, .msi, .deb etc) #33

Closed

malept added this to the Next major or minor version after 5.2.1 milestone Feb 17, 2016

develar force-pushed the ignore-out-path branch from 1b1d8d2 to 31487c8 Compare February 22, 2016 07:15

develar mentioned this pull request Feb 22, 2016

Document new API electron-userland/electron-builder#162

Merged

pyro2927 and others added 4 commits February 28, 2016 16:43

Automatically ignoring --out path

5147a47

Automatically ignoring --out path — Fix #64 (out dir equals to workin…

469a587

…g directory and we exclude previous artifacts directories)

Avoid manual Object.create in each test, fix createInferTest side eff…

cfe00a9

…ect — version property was deleted from prototype

move archs and platforms from index.js/test util.js to common.js

6623c25

develar force-pushed the ignore-out-path branch from 31487c8 to 6623c25 Compare February 28, 2016 15:44

malept changed the title ~~Automatically ignoring --out path~~ Automatically ignore --out path Feb 29, 2016

malept added a commit that referenced this pull request Feb 29, 2016

Merge pull request #255 from develar/ignore-out-path

d49b932

Automatically ignore --out path Closes #155.

malept merged commit d49b932 into electron:master Feb 29, 2016

develar deleted the ignore-out-path branch February 29, 2016 06:46

develar mentioned this pull request Feb 29, 2016

Ability to set tmpdir to false to disable using temp directory #276

Merged

malept mentioned this pull request Mar 8, 2016

--out should be added to the ignore list by default #120

Closed

Uh oh!

Automatically ignore --out path #255

Automatically ignore --out path #255

Uh oh!

Conversation

develar commented Feb 5, 2016

Uh oh!

malept commented Feb 5, 2016

Uh oh!

develar commented Feb 5, 2016

Uh oh!

develar commented Feb 5, 2016

Uh oh!

develar commented Feb 5, 2016

Uh oh!

develar commented Feb 7, 2016

Uh oh!

malept commented Feb 7, 2016

Uh oh!

develar commented Feb 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

develar commented Feb 9, 2016

Uh oh!

develar commented Feb 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

develar commented Feb 18, 2016

Uh oh!

malept commented Feb 18, 2016

Uh oh!

malept commented Feb 18, 2016

Uh oh!

develar commented Feb 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants