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.

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

Image Importer Improvements #4752

Merged
merged 2 commits into from
Jan 5, 2015
Merged

Image Importer Improvements #4752

merged 2 commits into from
Jan 5, 2015

Conversation

ErisDS
Copy link
Member

@ErisDS ErisDS commented Jan 2, 2015

ref #4608, #4609, #4690

  • fix errors with cleaning up files
  • improve handling of base directories, and introduce a simple valid format for zips (must contain importable files or folders, and may contain up to one base directory)
  • vastly improve test coverage

@ErisDS
Image Importer Improvements
add4c6b 
ref TryGhost#4608, TryGhost#4609, TryGhost#4690

- fix errors with cleaning up files
- improve handling of base directories, and introduce a simple valid format for zips (must contain importable files or folders, and may contain up to one base directory)
- vastly improve test coverage
@ErisDS
Copy link
Member Author

ErisDS commented Jan 3, 2015

Note: This PR comes before #4692, as it is a follow up to #4690 - will rebase the markdown one on this one.

jaswilli
jaswilli reviewed Jan 3, 2015
View reviewed changes
core/server/data/importer/index.js
this.getExtensionGlob(this.getExtensions(), ALL_DIRS), {cwd: directory}
);
if (extMatchesAll.length < 1 || extMatchesAll[0].split('/') < 1) {
return Promise.resolve(new errors.ValidationError('Invalid zip file: base directory read failed'));

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@jaswilli
Copy link
Contributor

jaswilli commented Jan 3, 2015

As I was going through this I noticed a couple methods in importer/index.js that are returning promises when they don't need to (isValidZip and getBaseDirectory). If they're switched to returning values/throwing errors it can make processZip and loadFile a little more straightforward and efficient.

I put up the changes here: https://github.com/jaswilli/Ghost/commit/ec6698fa720983fad7b25af5abe29096ac4f448b

Use or ignore 😄. I know you're trying to get to a destination that's further down the road than this PR--just playing around with this and thought I'd toss it out there.

@ErisDS
Copy link
Member Author

ErisDS commented Jan 3, 2015

Looks good to me - at one point I had branching code where the promises made more sense - but this is tonnes better now, just cherry picked your work into the PR 😜

jaswilli added a commit that referenced this pull request Jan 5, 2015
@jaswilli
Merge pull request #4752 from ErisDS/importer-updates
7f753ac 
Image Importer Improvements
@jaswilli jaswilli merged commit 7f753ac into TryGhost:master Jan 5, 2015
@ErisDS ErisDS deleted the importer-updates branch February 28, 2015 13:31
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
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ErisDS @jaswilli