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

feat(dataset): make dataset providers plugins #3055

Merged
merged 6 commits into from Jul 28, 2022

Conversation

olevski
Copy link
Member

@olevski olevski commented Jul 26, 2022

As I am working through #2971 one of the subtasks is to make the dataset providers plugins.

I isolated the changes for this and thought it would be a good idea to submit this PR separately from the rest.

Also wanted to get a feeling of whether my way of going about this is acceptable. I decided to not try to separate ProviderApi and the similar importer and exporter abstract classes and try to move some portion of them in the domain_model. But if you think this is necessary then I am happy to do it.

For now the plugins simply provide the defintion of a specific ProviderApi. And the provider factory is now using pluggy to list all the providers rather than importing what is only locally available.

@olevski olevski requested a review from a team as a code owner July 26, 2022 21:20
@olevski olevski force-pushed the 2971-make-dataset-providers-plugins-alt branch 2 times, most recently from 998a29b to 14f0cf0 Compare July 26, 2022 21:31
@olevski olevski force-pushed the 2971-make-dataset-providers-plugins-alt branch from 14f0cf0 to 72b4c16 Compare July 26, 2022 21:34
Panaetius
Panaetius previously approved these changes Jul 27, 2022
Copy link
Member

@Panaetius Panaetius left a comment

Choose a reason for hiding this comment

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

Looks all great. I left on comment on how we could simplify it by adding the method on the provider directly (then we don't need extra classes), but that doesn't change anything functionality-wise, so feel free to ignore.

renku/core/dataset/providers/dataverse.py Outdated Show resolved Hide resolved
@olevski olevski force-pushed the 2971-make-dataset-providers-plugins-alt branch from b37b075 to 3abe065 Compare July 27, 2022 12:50
@olevski olevski requested a review from Panaetius July 27, 2022 13:18
Panaetius
Panaetius previously approved these changes Jul 27, 2022
Copy link
Member

@Panaetius Panaetius left a comment

Choose a reason for hiding this comment

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

Thank you, glad it worked with the classmethod 🙂

@olevski
Copy link
Member Author

olevski commented Jul 28, 2022

@Panaetius can I get a re-approve here? There were merge conflicts I had to resolve and that cleared the approval.

@olevski olevski merged commit b68a8bb into develop Jul 28, 2022
@olevski olevski deleted the 2971-make-dataset-providers-plugins-alt branch July 28, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants