Skip to content

Fix/451 (WIP?) #452

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
Merged
mfeurer merged 5 commits into from
Jun 8, 2018
Merged

Fix/451 (WIP?) #452

mfeurer merged 5 commits into from
Jun 8, 2018

Conversation

@PGijsbers
Copy link
Collaborator

@PGijsbers PGijsbers commented Apr 23, 2018

Worked on: #451

  • Functions run_model_on_task and run_flow_on_task now allow both ordering of the first two arguments ((task, model) or (model, task) and (task, flow) or (flow, task), respectively).

  • Added two simple unit tests which test that the normal output with swapped arguments still works.

There are some outstanding questions, see my last comment in #451. I propose we continue the discussion there.

I made an early pull request to check with CI (I have some issues with my local environment atm and don't have the time to fix it).

@PGijsbers
Copy link
Collaborator Author

PGijsbers commented Apr 23, 2018

note: AppveyorCI shouldn't work on this branch because it does not have the updated scripts.

mfeurer
mfeurer reviewed Apr 23, 2018
View reviewed changes
Copy link
Collaborator

@mfeurer mfeurer left a comment

Choose a reason for hiding this comment

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

Thanks a lot. It would be great if you could already swap the default order (i.e. the argument names and their documentation) and add a TODO which reminds us to deprecate the argument swapping. Also, you can most likely simplify the unit tests quite a lot by mocking out functions which are not related to swapping the arguments. Then you only need to check if the mock objects got called correctly.

@PGijsbers
Copy link
Collaborator Author

PGijsbers commented May 29, 2018

Adding a TODO where? Open an issue or a #TODO in the code? Also, I can just add a deprecation warning right now instead. It doesn't really take an effort, but what should the text be? Something like

warnings.warn("The old argument order (task, model) is deprecated and will not be supported in the future"
              "Please use the order (model, task).", PendingDeprecationWarning)

whenever arguments are passed by the old order?

Again I'm very sorry this took so long. I learned not to take up a new issues right before a vacation because there's always a lot to catch up to when you get back anyway ^^;

@mfeurer
Copy link
Collaborator

mfeurer commented May 30, 2018

Yep, adding a #TODO as a comment was what I meant. And the deprecation warning would be great.

mfeurer
mfeurer approved these changes May 30, 2018
View reviewed changes
Copy link
Collaborator

@mfeurer mfeurer left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks a lot.

@mfeurer
Copy link
Collaborator

mfeurer commented May 30, 2018

Could you please resolve the conflicts and check the failing tests?

@mfeurer
Copy link
Collaborator

mfeurer commented Jun 8, 2018

Thanks! Test failures are due to missing tags on the test server. I will contact @janvanrijn about this.

@mfeurer mfeurer merged commit 4603345 into develop Jun 8, 2018
@mfeurer mfeurer deleted the Fix/451 branch June 8, 2018 11:35
This was referenced Jun 8, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mfeurer mfeurer mfeurer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@PGijsbers @mfeurer