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

Leave config file only for general importers #463

Merged
merged 1 commit into from
Feb 7, 2018

Conversation

lorenaqendro
Copy link
Contributor

Fixes #420

Description

Context: When creating the general csv importer, there was a need for a configuration file where to specify how to interpret the dataset and other info so it could be used in the DC data structure (see example). At the time, we decided that it made sense to have a configuration file for each importer not only the general ones. This way we could leave to the user the decision of personalising the built-in importer via the config file. This meant of course changes in the present importers we did not implement. What seemed to be a good design decision at the time (might still be) was not adopted in practice.

In this PR I left the config file only for the general importers and removed it from the rest. I created an Abstract class to represent the importers that have config files so it should be ok if we want it for future importers.

Checklist

  • []Created new test(s) (if applicable)
  • [ ]Updated the README / documentation (if applicable)

Copy link
Contributor

@arya-hemanshu arya-hemanshu left a comment

Choose a reason for hiding this comment

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

Looks good, just one question, why creating another Abstract class rather adding another constructor to already existing Abstract class?

@lorenaqendro
Copy link
Contributor Author

@arya-hemanshu I did it so we can create a kind of hierarchy for the general importers or better the ones that provide a config file. This is will be more useful when we have other general importers of course.

Copy link
Contributor

@arya-hemanshu arya-hemanshu left a comment

Choose a reason for hiding this comment

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

Good to merge

@lorenaqendro lorenaqendro merged commit 2a72ebc into master Feb 7, 2018
@lorenaqendro lorenaqendro deleted the lq-420-remove_config_file branch February 7, 2018 17:18
@lorenaqendro lorenaqendro added this to the Release Candidate milestone Mar 14, 2018
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