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

CLN: Remove Class trait #520

Merged
merged 5 commits into from
Aug 29, 2019
Merged

CLN: Remove Class trait #520

merged 5 commits into from
Aug 29, 2019

Conversation

skailasa
Copy link
Contributor

Addresses #351, removing Class trait, and adding an alias Subclass as suggested for the Type trait

@skailasa skailasa changed the title Remove Class trait #351 Remove Class trait Aug 28, 2019
@skailasa
Copy link
Contributor Author

Getting a strange failure on CI, unsure of whether or not it's due to my changes.

Unexpected error: BadZipfile('File is not a zip file',)
381

@mdickinson
Copy link
Member

unsure of whether or not it's due to my changes

Looks fairly unlikely: the failing job hadn't even got as far as installing Traits. I've restarted the failing job to see if the problem goes away. If it turns out to be reproducible, we'll have to dig in further.

@codecov-io
Copy link

codecov-io commented Aug 28, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #520   +/-   ##
=========================================
  Coverage          ?   65.29%           
=========================================
  Files             ?       44           
  Lines             ?     7045           
  Branches          ?     1414           
=========================================
  Hits              ?     4600           
  Misses            ?     2022           
  Partials          ?      423
Impacted Files Coverage Δ
traits/trait_types.py 63.5% <ø> (ø)
traits/api.py 87.87% <100%> (ø)

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 8eede3b...a4e5908. Read the comment docs.

@skailasa skailasa changed the title Remove Class trait CLN: Remove Class trait Aug 28, 2019
@mdickinson
Copy link
Member

Thank you for doing this. A couple of comments:

I'm not fully convinced by the need for the Subclass alias; can we defer that to a separate PR (so that I can argue with @corranwebster about it on that PR)? If we do introduce Subclass, I think we probably want to simply do Subclass = Type (so that it's a genuine alias for Type); we'd also need to add documentation and tests for that alias. For this PR, I'd suggest leaving Subclass out.

That leaves the Class removal; I agree that it's fine to do this without a deprecation warning, since no-one should be using this anyway. I'll aim to get this PR into Traits 6.0.0, which will be the first release not supporting Python 2.

The Class trait type is documented in docs/source/traits_api_reference/trait_types.rst and in docs/source/traits_user_manual/defining.rst; please could you remove those doc references?

It would make sense to get rid of ListClass and ListInstance (as well as their documentation) as part of this same PR: they also have to do with old-style classes.

@mdickinson
Copy link
Member

I've restarted the failing job to see if the problem goes away.

Looks like it did. 🎉

@corranwebster
Copy link
Contributor

I agree with @mdickinson on if we have Subclass that Subclass = Type and be done with it.

I'm also reasonably OK with Subclass as a concept - it has a nice parallel with Instance : isinstance :: Subclass : issubclass

@mdickinson
Copy link
Member

mdickinson commented Aug 29, 2019

it has a nice parallel with Instance : isinstance :: Subclass : issubclass

True. My first thought was that Type probably doesn't correspond to issubclass in the presence of ABCs and the like, but apparently it does:

>>> from traits.api import *
>>> from numbers import Integral
>>> class A(HasTraits):
...     int_type = Type(Integral)
... 
>>> a = A()
>>> a.int_type = float
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mdickinson/.edm/envs/traits-test-3.6/lib/python3.6/site-packages/traits/trait_types.py", line 3354, in validate
    self.error(object, name, value)
  File "/Users/mdickinson/.edm/envs/traits-test-3.6/lib/python3.6/site-packages/traits/trait_handlers.py", line 236, in error
    object, name, self.full_info(object, name, value), value
traits.trait_errors.TraitError: The 'int_type' trait of an A instance must be a subclass of Integral or None, but a value of <class 'float'> <class 'type'> was specified.
>>> a.int_type = int 
>>> a.int_type = np.int64

Given that, Subclass conveys the behaviour more accurately than Type (which you might expect to force a specific type, rather than something satisfying an issubclass check). So perhaps for Traits 6.0.0 we should rename Type to Subclass and keep Type as a backwards compatibility alias?

In any case, please let's do all the subclass-related stuff in a separate PR.

@@ -277,8 +277,6 @@ the table.
| CArray | CArray( [*dtype* = None, *shape* = None, *value* = None, |
| | *typecode* = None, \*\*\ *metadata*] ) |
+------------------+----------------------------------------------------------+
| Class | Class( [*value*, \*\*\ *metadata*] ) |
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the Class entry from the index above this table, too.

@mdickinson
Copy link
Member

@skailasa Please could you remove ListInstance and ListClass and their docs, too?

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. Thank you!

@mdickinson mdickinson merged commit e2b8106 into enthought:master Aug 29, 2019
@rahulporuri rahulporuri mentioned this pull request Nov 18, 2019
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.

4 participants