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

Refactor / experiment init #1332

Merged

Conversation

WilliamHPNielsen
Copy link
Contributor

In the spirit of #1323, this PR proposed to do the same for the Experiment class

Changes proposed in this pull request:

  • Refactor the __init__ of Experiment so that no _new method exists
  • Add a test for the new behaviour
  • Fix two broken properties of the Experiment (started_at and finished_at)

@astafan8 @QCoDeS/core

@codecov
Copy link

codecov bot commented Oct 23, 2018

Codecov Report

Merging #1332 into master will increase coverage by 0.02%.
The diff coverage is 76.47%.

@@            Coverage Diff             @@
##           master    #1332      +/-   ##
==========================================
+ Coverage   72.55%   72.57%   +0.02%     
==========================================
  Files          74       74              
  Lines        8569     8577       +8     
==========================================
+ Hits         6217     6225       +8     
  Misses       2352     2352

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

awesome work! just some minor things to do in my opinion, see comments


def _new(self,
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, there was no problem with the _new method. It has _, hence noone should call from outside of the class. But that was exactly the problem :) anyway, of you feel like, please don't hesitate to encapsulate the else-clause part of the logic into a private method in order to increase readability of the __init__ method.

@WilliamHPNielsen WilliamHPNielsen merged commit 3a19267 into microsoft:master Oct 24, 2018
@WilliamHPNielsen WilliamHPNielsen deleted the refactor/experiment_init branch October 24, 2018 09:22
giulioungaretti pushed a commit that referenced this pull request Oct 24, 2018
Merge: a6d4c22 bc54dde
Author: William H.P. Nielsen <[email protected]>

    Merge pull request #1332 from WilliamHPNielsen/refactor/experiment_init
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