Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
malept merged 1 commit into from
Mar 5, 2016
Merged

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

malept merged 1 commit into from
Mar 5, 2016

Conversation

@develar
Copy link
Contributor

@develar develar commented Feb 29, 2016

Continue #251.

Test added.

develar
develar reviewed Feb 29, 2016
View reviewed changes
common.js
return
}

fs.move(tempPath, finalPath, function (err) {
Copy link
Contributor Author

@develar develar Feb 29, 2016

Choose a reason for hiding this comment

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

fs-extra handles EPERM on windows correctly. We don't need to use mkdirp/ncp here explicitly.

@malept
Copy link
Member

malept commented Feb 29, 2016

Is this ready for review now? I don't want to merge this before you're ready (again).

@develar
Copy link
Contributor Author

develar commented Feb 29, 2016

I suppose #251 was closed automatically because was rebased to the same commits as #255 by GitHub, not by you.

Yes, please review.

@malept malept added this to the 6.0.0 milestone Feb 29, 2016
malept
malept reviewed Feb 29, 2016
View reviewed changes
cli.js
if (args.tmpdir === 'false') {
args.tmpdir = false
}

Copy link
Member

@malept malept Feb 29, 2016

Choose a reason for hiding this comment

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

Should add a comment that minimist doesn't do this for us. I almost forgot why this conditional was added.

Copy link
Member

@malept malept Feb 29, 2016

Choose a reason for hiding this comment

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

Please also add documentation to readme.md and usage.txt about this new functionality to tmpdir. Though you may want to hold off on that until #275 is merged.

@malept malept added the docs-needed 📝 Pull request needs documentation label Feb 29, 2016
@develar develar force-pushed the add-use-temp-dir-option branch 2 times, most recently from 35f6735 to 203daff Compare March 1, 2016 07:14
develar
develar reviewed Mar 1, 2016
View reviewed changes
cli.js Outdated
@@ -22,6 +22,11 @@ if (!args.dir || (!args.all && (!args.platform || !args.arch))) {
process.exit(1)
}

// minimist doesn't support to specify several types (string|boolean)
Copy link
Contributor Author

@develar develar Mar 1, 2016

Choose a reason for hiding this comment

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

Commend added.

Copy link
Member

@malept malept Mar 1, 2016

Choose a reason for hiding this comment

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

It would read better as:

// minimist doesn't support multiple types for a single argument (in this case, `String` or `false`)

Copy link
Contributor Author

@develar develar Mar 1, 2016

Choose a reason for hiding this comment

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

Fixed, thanks.

On Mar 1, 2016, at 8:22 AM, Mark Lee [email protected] wrote:

In cli.js
:

@@ -22,6 +22,11 @@ if (!args.dir || (!args.all && (!args.platform || !args.arch))) {
process.exit(1)
}

+// minimist doesn't support to specify several types (string|boolean)
It would read better as:

// minimist doesn't support multiple types for a single argument (in this case, String or false)

—
Reply to this email directly or view it on GitHub
.

@develar develar force-pushed the add-use-temp-dir-option branch 2 times, most recently from 0c12b8b to d34bc04 Compare March 1, 2016 07:37
develar
develar reviewed Mar 1, 2016
View reviewed changes
readme.md Outdated
@@ -248,7 +248,7 @@ If the file extension is omitted, it is auto-completed to the correct extension

`tmpdir` - *String*

The base directory to use as a temp directory. Defaults to the system temp directory.
The base directory to use as a temp directory. Defaults to the system temp directory. Set to `false` to write to `out` directly without temp.
Copy link
Contributor Author

@develar develar Mar 1, 2016

Choose a reason for hiding this comment

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

Docs added.

@develar develar force-pushed the add-use-temp-dir-option branch 2 times, most recently from 5985c9a to 1362bc3 Compare March 1, 2016 08:28
malept
malept reviewed Mar 1, 2016
View reviewed changes
usage.txt Outdated
@@ -33,7 +33,7 @@ sign should contain the identity to be used when running `codesign
sign-entitlements the path to entitlements used in signing (mas platform only)
strict-ssl whether SSL certificates are required to be valid when downloading Electron.
It defaults to true, use --strict-ssl=false to disable checks.
tmpdir temp directory. Defaults to system temp directory.
tmpdir temp directory. Defaults to system temp directory. Set to `false` to write to `out` directly without temp.
Copy link
Member

@malept malept Mar 1, 2016

Choose a reason for hiding this comment

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

For consistency:

Defaults to system temp directory, use --tmpdir=false to disable functionality.

Copy link
Contributor Author

@develar develar Mar 1, 2016

Choose a reason for hiding this comment

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

Changed, but don't quite agree that "to disable functionality." is clear — because result of "disable functionality." is not clear. Opposite to "to write to out directly without temp".

Copy link
Member

@malept malept Mar 1, 2016

Choose a reason for hiding this comment

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

Perhaps:

Specify --tmpdir=false to disable use of a temporary directory.

Copy link
Contributor Author

@develar develar Mar 1, 2016

Choose a reason for hiding this comment

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

I feel a little stupid compared to you :) Changed.

Copy link
Member

@malept malept Mar 1, 2016

Choose a reason for hiding this comment

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

Well, I was trying to avoid having it say some variant of "temp directory" three times in three sentences - I guess that didn't work out. (I am open to suggestions.)

@malept malept removed the docs-needed 📝 Pull request needs documentation label Mar 1, 2016
@develar develar force-pushed the add-use-temp-dir-option branch from 1362bc3 to 75bdb0c Compare March 1, 2016 16:47
@develar
Ability to set tmpdir to false to disable using temp directory
297345b 
And as a workaround of concurrent access to the same temp dir
@develar develar force-pushed the add-use-temp-dir-option branch from 75bdb0c to 297345b Compare March 1, 2016 16:56
@malept
Copy link
Member

malept commented Mar 5, 2016

👍

malept added a commit that referenced this pull request Mar 5, 2016
@malept
Merge pull request #276 from develar/add-use-temp-dir-option
ac5098c 
Ability to set tmpdir to false to disable using temp directory
@malept malept merged commit ac5098c into electron:master Mar 5, 2016
@develar develar deleted the add-use-temp-dir-option branch May 15, 2016 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

@develar @malept