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

Restructure importers/import managers... #14

Closed
mikesname opened this issue Dec 16, 2013 · 3 comments
Closed

Restructure importers/import managers... #14

mikesname opened this issue Dec 16, 2013 · 3 comments
Assignees
@mikesname
Labels
bug

Comments

@mikesname
Copy link
Contributor

The importer and import manager classes have a fair few warts, mostly going back to their earliest genesis as an EAD-specific hack:

  • CsvImportManager extends XmlImportManager
  • code dup between CsvImportManager and SaxImportManager
  • the ImportManager interface doesn't do much, and isn't really used
  • the importFile, importFiles signatures and overrides are pretty much just as confusing as when I first wrote them (sorry)

Now that we have a more generic import system it would be nice to tidy this up a bit before looking at things like issue #10.

I am happy to file this under "things to do when bored of hacking CSS", but don't want to tread on your toes @bencomp and @lindareijnhoudt. Thoughts?
@ghost ghost assigned mikesname Dec 16, 2013
@bencomp
Copy link
Contributor

bencomp commented Dec 16, 2013

In a branch "dates" I changed the hierarchy of some importers (I created a general EadHandler and made some existing CHI-specific importers extend that). It appears to work as expected, but I need to take another look and maybe add some other empty methods for getting specific data (currently identifier, title, date, language) before merging it in master. So perhaps you'd like to start from there. But let's set some priorities first. (First sleep :-))

@mikesname
Copy link
Contributor Author

Okay, I'll take a look at that branch.

@mikesname
Copy link
Contributor Author

I've merged your stuff here: c484b5f. There's a bunch of import test failures (incorrect node counts) but nothing expected.

bencomp added a commit to bencomp/ehri-rest that referenced this issue Jan 21, 2015
@bencomp
Refactor some Importers: XmlImporter -> MapImporter
b079d9e 
Made `AbstractImporter<T>` more generic by using `T` for all `importItem` and `extractDates`
methods.
(I wouldn't call it fully generic, because some of these have `Map<String, Object>` as
return type.)

The field `documentContext` of type `T` has been removed from `AbstractImporter<T>`,
because it was never used.

Renamed `XmlImporter<T>` to `MapImporter` (it's no longer generic), since there is no
relation to XML (see also issue EHRI#14) and it is the base implementation for importers of
`Map<String, Object>` representations of nodes. `XmlImporterTest` is `MapImporterTest`.

Subclasses of `MapImporter` no longer need to implement `extractDates(Object)`. These
methods have been removed.

Added or updated javadoc comments in several locations.
@bencomp bencomp mentioned this issue Jan 21, 2015
Merged
@bencomp bencomp mentioned this issue Jan 22, 2015
Merged
mikesname added a commit that referenced this issue Jan 22, 2015
@mikesname
Merge pull request #90 from bencomp/14-importer-refactor
9bd08e0 
Refactor ImporterManagers + tests (fixes #14)
@bencomp bencomp mentioned this issue Jan 23, 2015
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mikesname mikesname

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mikesname @bencomp