Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Move file-moving logic from tree-view's MoveDialog to fsPlus.moveSync #18

Merged
merged 10 commits into from
May 1, 2015
Merged

Conversation

oclbdk
Copy link
Contributor

@oclbdk oclbdk commented Apr 28, 2015

As far as I can tell, the tree-view's MoveDialog is the only client of fsPlus.moveSync -- https://github.com/atom/tree-view/blob/48fd001687f14bb8538fe6c92f59d9774fa87c4f/lib/move-dialog.coffee#L31. It adds a lot of custom logic (e.g. checking when a move is valid, creating the target's parent directory if it doesn't exist, etc) that could be useful for other callers.

I'm specifically making this move so we can reuse the function for remote file support.

@oclbdk
Copy link
Contributor Author

oclbdk commented Apr 28, 2015

Regarding the possibility of breaking existing packages that rely on fs.moveSync throwing an error if the target's parent directory doesn't exist (or the other side effects from fs.renameSync)... it's trivial to change their calls from fs.moveSync to fs.renameSync.

I see fs.moveSync as a smarter helper method that's more useful than fs.renameSync in the same way that fs.remove supersedes fs.unlink and fs.rmdir.

@oclbdk oclbdk closed this Apr 28, 2015
@oclbdk oclbdk reopened this Apr 28, 2015
@oclbdk
Copy link
Contributor Author

oclbdk commented Apr 28, 2015

I accidentally closed this pull request by hitting a keyboard shortcut, would still very much appreciate comments or a merge. :)

@@ -4,6 +4,7 @@ path = require 'path'

_ = require 'underscore-plus'
async = require 'async'
{Promise} = require 'es6-promise'
Copy link
Contributor

Choose a reason for hiding this comment

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

For things like this that might be built-in depending on the context, we try to use the global when available and fall back to the require when it isn't there.

So something like:

Promise = global.Promise ? require('es6-promise').Promise

@@ -523,4 +542,18 @@ lstatSyncNoException ?= (args...) ->
isPathValid = (pathToCheck) ->
pathToCheck? and typeof pathToCheck is 'string' and pathToCheck.length > 0

isMoveTargetValidSync = (source, target) ->
try
oldStat = fs.statSync(source)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line and the line below it should use statSyncNoException for speed 🐎

Basically it removes the need for a try/catch and instead you can just return true if either oldStat or newStat is false.

kevinsawicki added a commit that referenced this pull request May 1, 2015
Move file-moving logic from tree-view's MoveDialog to fsPlus.moveSync
@kevinsawicki kevinsawicki merged commit 5f10361 into atom:master May 1, 2015
@kevinsawicki
Copy link
Contributor

Thanks for this! 🚢

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants