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

implement TraitInstance to inherit directly from TraitHandler #761

Merged
merged 2 commits into from
Jan 17, 2020

Conversation

shoeb-github
Copy link
Contributor

contributes to #697

  • copied the methods fromThisClass to TraitInstance.

-- because ThisClass will be deprecated in version 6 release.
@shoeb-github shoeb-github self-assigned this Jan 16, 2020
@codecov-io
Copy link

codecov-io commented Jan 16, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@2e5a601). Click here to learn what that means.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #761   +/-   ##
=========================================
  Coverage          ?   71.73%           
=========================================
  Files             ?       51           
  Lines             ?     6337           
  Branches          ?     1281           
=========================================
  Hits              ?     4546           
  Misses            ?     1393           
  Partials          ?      398
Impacted Files Coverage Δ
traits/trait_handlers.py 58.48% <33.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e5a601...851ece6. Read the comment docs.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

Please remove the validate_none and info_none methods. Apart from that, this LGTM

@@ -419,6 +428,9 @@ def info(self):

return result

def info_none(self):
Copy link
Member

Choose a reason for hiding this comment

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

I think this method can be removed: info_none is only used in ThisClass.__init__.

def validate_failed(self, object, name, value):
self.error(object, name, value)

def validate_none(self, object, name, value):
Copy link
Member

Choose a reason for hiding this comment

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

This method isn't needed.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM, if the CI agrees.

@shoeb-github
Copy link
Contributor Author

Apart from validate_none and info_none methods, I also removed validate_failed method.

@mdickinson
Copy link
Member

I also removed validate_failed method.

Yep, I think that's reasonable. My only worry about that would be subclasses of TraitInstance that rely on validate_failed, but I can't see any place where TraitInstance is subclassed in ETS, and it's unlikely to be used at all outside ETS.

@shoeb-github shoeb-github merged commit df30dc5 into master Jan 17, 2020
@shoeb-github shoeb-github deleted the maint/TraitInstance-with-TrainHandler-base branch January 17, 2020 15:33
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.

3 participants