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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Importer #4637

Closed
wants to merge 3 commits into from
Closed

Refactor Importer #4637

wants to merge 3 commits into from

Conversation

ErisDS
Copy link
Member

@ErisDS ErisDS commented Dec 12, 2014

I've kept this as 3 commits for now, just so it's easier to see what changes I've made.

The first commit (88db807) is the main change, introducing a new /data/importer/ folder which contains a handlers folder and an importers folder. The new data/importer/data/ is just a hook into the old /data/import/ code - I want to do more to move this around and separate it into two parts: data preprocessing and data storage, but it isn't necessary to get the rest of the import changes so I'm leaving this half-done for now.

This restructure allows for the creation of other handlers and importers which manage other file types. I've done a lot of this locally, so I'm reasonably confident this refactor is the right direction.

The second change here removes the concept of versioning from importing. We don't actually use it at present and it just makes migrations more complex (see #4479)

Finally, I made a couple of changes to the data importer itself as it wasn't working for me properly. I found that if created_at wasn't set on the post, then an SQLITE error was being thrown but not passed all the way through (reproduce with this: https://gist.github.com/ErisDS/5eff62547f66177f8057). The changes in the 3rd commit both fix the underlying problem with errors not being passed through and also adds an extra line to the post importer which sets created_at if it is not already set (if it's set, it should be honoured, if it's not set, it should be considered to be now.

If I could get a bit of a review of these changes, that'd be grand, then I'll squash:)

@ErisDS ErisDS mentioned this pull request Dec 12, 2014
@ErisDS
Copy link
Member Author

ErisDS commented Dec 12, 2014

See #4638 for an example of how this can/will be extended

@ErisDS
Copy link
Member Author

ErisDS commented Dec 13, 2014

@jaswilli, @sebgie, @jgable and anyone else who might be around - I would love a little feedback on this - if there's nothing glaringly horrible about it then it would be good to get it merged so I can complete the image and markdown importers (and tests 😉)

@sebgie
Copy link
Contributor

sebgie commented Dec 13, 2014

Looks good to me :).

I think there is a bug when uploading a corrupt import file which causes the application to hang.

Steps to reproduce:

  • upload invalid file (I deleted the db property)
  • visit the content screen
  • application hangs

- fixup a couple of issues with the data importer
@ErisDS
Copy link
Member Author

ErisDS commented Dec 13, 2014

Found & Fixed ;)

@ErisDS
Copy link
Member Author

ErisDS commented Dec 14, 2014

After having come back to this to add tests, I felt it made more sense as two units of work - one with the refactor + new tests, and one for the improvement work which includes removing versioning and fixing a couple of bugs.

Therefore, I'm going to close this PR in favour of #4646, which is just the refactor. Improvements PR coming soon.

@ErisDS ErisDS closed this Dec 14, 2014
@ErisDS ErisDS deleted the refactor-4605 branch January 12, 2015 14:20
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.

2 participants