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 add_class_trait in the presence of subclasses #1461

Merged
merged 7 commits into from
May 17, 2021

Conversation

mdickinson
Copy link
Member

This PR fixes the issue described in #1460, and adds some tests for add_class_trait. Prior to this PR, there were no tests at all for add_class_trait.

The PR also does a drive-by fix of a bug involving prefix traits and an invalid use of sort, leading to the error TypeError: sort() takes no positional arguments.

Unfortunately, one of the newly added tests is failing as part of the full test suite, but passing standalone. It appears that we have a test interaction - some test somewhere in the test suite is mutating the class traits on HasTraits. I'll try to track it down.

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference) N/A
  • Update User manual (docs/source/traits_user_manual) N/A
  • Update type annotation hints in traits-stubs N/A

@mdickinson
Copy link
Member Author

I'll try to track it down.

The culprit is test_abc.py, in combination with issue #58:

HasTraits(x=10)

@mdickinson
Copy link
Member Author

The documentation build issue that's causing the CI to fail on this PR should be fixed in #1462.

@rahulporuri rahulporuri self-requested a review May 13, 2021 03:54
traits/has_traits.py Outdated Show resolved Hide resolved
Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

While orthogonal to main idea of the PR, there aren't tests for the is_subclass behaviour where there is an error as described in the new docstring for _add_class_trait.

Otherwise LGTM.

@mdickinson
Copy link
Member Author

there aren't tests for the is_subclass behaviour where there is an error

Yes, that should be fixed. (Though the use of TraitError here bugs me: it feels like the wrong exception, and writing a test for it bakes in that wrongness a little bit harder. That's not a good excuse not to write that test, though.)

@mdickinson
Copy link
Member Author

writing a test for it bakes in that wrongness a little bit harder

Though I guess I've already done that by documenting it. :-(

@mdickinson
Copy link
Member Author

there aren't tests for the is_subclass behaviour where there is an error

Tests added.

mdickinson and others added 2 commits May 17, 2021 10:11
@mdickinson mdickinson merged commit 5a04454 into master May 17, 2021
@mdickinson mdickinson deleted the fix/add-class-trait-has-items branch May 17, 2021 09:19
@rahulporuri rahulporuri removed their request for review May 17, 2021 10:12
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