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

Fix unclarity about post_delay/inter_delay of Parameter, remove deprecated get/set delay and get/set step #1387

Merged
merged 5 commits into from
Nov 19, 2018

Conversation

astafan8
Copy link
Contributor

@astafan8 astafan8 commented Nov 19, 2018

Parameter's post_delay and inter_delay properties have confusing descriptions. This PR addresses this.

Also remove deprecated get_delay, set_delay, get_step, set_step. Make delay kwarg a noop.

Discovered by @damazter

@astafan8 astafan8 added the docs Related to docs improvements label Nov 19, 2018
@codecov
Copy link

codecov bot commented Nov 19, 2018

Codecov Report

Merging #1387 into master will increase coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1387      +/-   ##
==========================================
+ Coverage   73.38%   73.46%   +0.07%     
==========================================
  Files          79       79              
  Lines        9282     9266      -16     
==========================================
- Hits         6812     6807       -5     
+ Misses       2470     2459      -11

Copy link
Collaborator

@jenshnielsen jenshnielsen 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. We could also remove the deprecated delay and it's getters and setters to clear up. They have been deprecated for long enough I think

@astafan8
Copy link
Contributor Author

@jenshnielsen Do you mean set_delay/get_delay? Shall i do it in this PR?

Shall I also remove set_step/get_step ?

@jenshnielsen
Copy link
Collaborator

Yes set/get delay and make the delay kwarg a noop. Fine to remove the get/set step too I think

@astafan8 astafan8 changed the title Fix unclarity about post_delay and inter_delay of Parameter Fix unclarity about post_delay/inter_delay of Parameter, remove deprecated get/set delay and get/set step Nov 19, 2018
@astafan8
Copy link
Contributor Author

@jenshnielsen Could you confirm that i did the removal of deprecated things right? and the noop of delay?

@jenshnielsen
Copy link
Collaborator

@astafan8 I think this looks great. If we are extra paranoid we could make the code raise if you pass delay as a kwarg but I will not insist that is needed

@astafan8 astafan8 merged commit 4140df2 into microsoft:master Nov 19, 2018
@astafan8 astafan8 deleted the bug/parameter_delays branch November 19, 2018 15:42
giulioungaretti pushed a commit that referenced this pull request Nov 19, 2018
Merge: bebb6d5 38b3f87
Author: Mikhail Astafev <astafan8@gmail.com>

    Merge pull request #1387 from astafan8/bug/parameter_delays
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Related to docs improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants