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

Feature/api dataset load args #1445

Closed
wants to merge 3 commits into from
Closed

Feature/api dataset load args #1445

wants to merge 3 commits into from

Conversation

lucasjamar
Copy link
Contributor

@lucasjamar lucasjamar commented Apr 17, 2022

Description

In response to #711

lucasjamar and others added 2 commits April 17, 2022 22:59
Signed-off-by: lucas.jamar <lucas.jamar@alpiq.com>
@lucasjamar lucasjamar requested a review from idanov as a code owner April 17, 2022 21:20
@datajoely
Copy link
Contributor

I like this! Would you mind adding a unit test?

@antonymilne
Copy link
Contributor

Should we move all the other optional arguments like data, method, etc. inside load_args also? This would be a breaking change so need to go in develop (and we can keep your PR going to main). url should stay top-level though.

This would make for a more consistent interface and be tidier I think, but interested in hearing what others think. Maybe there's a good reason those arguments are more important than others passed to load_args and belong at top-level?

Signed-off-by: lucas.jamar <lucas.jamar@alpiq.com>
@merelcht
Copy link
Member

merelcht commented Jun 7, 2022

Hi @lucasjamar, are you still interested in completing this PR? Let the team know if you need any help! 🙂

@lucasjamar
Copy link
Contributor Author

@MerelTheisenQB . Im sorry but i really cant find the time to do this properly. I wold be happy if someone can take this over and get the cicd checks to pass. Sorry

@merelcht
Copy link
Member

merelcht commented Jun 8, 2022

@MerelTheisenQB . Im sorry but i really cant find the time to do this properly. I wold be happy if someone can take this over and get the cicd checks to pass. Sorry

Thanks for the update @lucasjamar and no worries. We'll add it to our backlog and also welcome other contributors to take over if they have capacity.

@merelcht merelcht added the Help Wanted 🙏 Contribution task, outside help would be appreciated! label Jun 8, 2022
@antonymilne
Copy link
Contributor

Superseded by #1633. Thanks @lucasjamar for your initial work for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted 🙏 Contribution task, outside help would be appreciated!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants