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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Fix mozilla/thimble.mozilla.org#889: Remove root folder thimble-project if exists #724

Closed
wants to merge 3 commits into from

Conversation

sdalmeida
Copy link

@sdalmeida sdalmeida commented Apr 18, 2017

The user is now able to export and import a zipped project without creating redundant folders (As first described here: mozilla/thimble.mozilla.org#889).

Because ArchiveUtils.js is being changed by @humphd, I've added the [WIP] in the title. Once his PR lands, I'll make the necessary changes 馃憤

Copy link

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Looks pretty good, but I haven't tested it yet. Can you fix the things I mention here?

Also, two more things:

  • We should probably change the name of the .zip file we produce to thimble-project.zip vs. project.zip if it isn't already (can't remember).
  • Can you file a bug to make this string configurable on startup? Bramble might get used outside of Thimble, so it's not ideal to hard-code that string in here. We'd probably want to add this to src/bramble/client/main.js such that you can override what it uses in the constructor for Bramble.

isDirectory: isDir,
data: isDir ? null : new Buffer(file.asArrayBuffer())
});
});

function removeProjectFolder(path){
// Nuke root folder thimble-project/ if exists
var regex = /^thimble\-project\//g;
Copy link

Choose a reason for hiding this comment

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

Let's combine these two lines:

return path.replace(/^thimble\-project\//, "");

Also, I don't think you want to include the g in your regex, since you're only wanting to deal with the case of this string starting at the beginning.

isDirectory: isDir,
data: isDir ? null : new Buffer(file.asArrayBuffer())
});
});

function removeProjectFolder(path){
Copy link

Choose a reason for hiding this comment

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

removeThimbleProjectFolder is probably more accurate a name.

isDirectory: isDir,
data: isDir ? null : new Buffer(file.asArrayBuffer())
});
});

function removeProjectFolder(path){
// Nuke root folder thimble-project/ if exists
Copy link

Choose a reason for hiding this comment

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

Expand this comment slightly:

// Nuke root folder `thimble-project/` if exists so that project zip files
// can be re-imported without adding an unnecessary folder.

@sdalmeida
Copy link
Author

sdalmeida commented Apr 24, 2017

I did a rebase on my code and I've added the changes you asked for. Going to file a new issue for the string you asked.

Just to recap, you want the string "thimble-project.zip" and "thimble-project" to be reconfigurable right?

@humphd
Copy link

humphd commented May 12, 2017

@Simon66 you still finishing this?

@humphd
Copy link

humphd commented Jun 7, 2017

I'm finishing this in #789

@humphd humphd closed this Jun 7, 2017
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.

None yet

2 participants