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

Apply various stubs fixes #1718

Merged
merged 5 commits into from
Oct 3, 2022
Merged

Apply various stubs fixes #1718

merged 5 commits into from
Oct 3, 2022

Conversation

mdickinson
Copy link
Member

This PR applies various stub fixes; these fixes bring us closer to getting the stubs tests passing on an editable install.

traits/has_traits.pyi Outdated Show resolved Hide resolved
@@ -152,8 +152,7 @@ class Vetoable(HasStrictTraits):

VetoableEvent: _Any

class MetaInterface(ABCMetaHasTraits):
def __call__(self, adaptee: _Any, default: _Any = ...): ...
class MetaInterface(ABCMetaHasTraits): ...
Copy link
Member Author

Choose a reason for hiding this comment

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

The __call__ signature was causing complaints about disagreement with the parent class. The plan for 7.0 is to get rid of the (long deprecated) special behaviour of MetaInterface.__call__ anyway, but I'll do that in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do that in a separate PR.

See #1719

obj.var = [1, 2, "3"]
obj.var = None
obj.var = ['1']
obj.i = {"a": 5, "b": 6}
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know whether these .var assignments were intentional, but I'd be happy to take a leaf out of NumPy's book and have our initial stubs support recommended use-cases instead of all possible use-cases. In this particular case, most code should be using HasStrictTraits rather than HasTraits.

obj.var = None # E: assignment

obj.var = [1, 2, 3]
obj2 = TestAnyList()
Copy link
Member Author

Choose a reason for hiding this comment

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

Case where mypy (legitimately) didn't like the fact that we re-used the same variable name obj for something of a different type.

@@ -21,6 +21,6 @@ class HasName(HasStrictTraits):

def try_assigning_age(x: HasName, new_name: str):
try:
x.age = new_name
x.name = new_name
Copy link
Member Author

Choose a reason for hiding this comment

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

This was my code; I have no idea what I was thinking.

Copy link
Contributor

Choose a reason for hiding this comment

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

and we will never know. until Enthought succeeds in creating the time machine that is.

@@ -108,7 +108,7 @@ def run_mypy(filepath):
dest_filename = os.path.basename(filepath)
dest = shutil.copyfile(filepath, os.path.join(tempdir, dest_filename))
normal_report, error_report, exit_status = mypy_api.run(
[dest, '--show-error-code'])
[dest, '--show-error-code', '--follow-imports=skip'])
Copy link
Member Author

Choose a reason for hiding this comment

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

The extra argument here was necessary to prevent errors from files other than the one under test. Eventually we should be able to fix the main Traits codebase up enough that we no longer need this, but it may take some time.

@@ -165,5 +165,7 @@ def assertRaisesMypyError(self, filepath):
for line, error_codes in parsed_mypy_output.items():
if line not in line_error_map or sorted(
line_error_map[line]) != sorted(list(error_codes)):
s = "\n{}\n{}".format(str(filepath), normal_report)

s = "Unexpected error on line {}\n{}\n{}".format(
Copy link
Member Author

Choose a reason for hiding this comment

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

Drive-by improvement to report the line number that caused a test to fail. (Otherwise it's hard to understand what went wrong from the test output.)

@@ -28,9 +28,6 @@ class _TraitMixin(Generic[_Accepts, _Stores]):
def __set__(self, object: Any, value: _Accepts) -> None: ...


_Trait = _TraitMixin()
Copy link
Member Author

Choose a reason for hiding this comment

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

I can't tell what this was for, and mypy was complaining about _Trait needing a type declaration, so I've deleted it.

@rahulporuri rahulporuri self-requested a review September 20, 2022 09:11
Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -21,6 +21,6 @@ class HasName(HasStrictTraits):

def try_assigning_age(x: HasName, new_name: str):
try:
x.age = new_name
x.name = new_name
Copy link
Contributor

Choose a reason for hiding this comment

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

and we will never know. until Enthought succeeds in creating the time machine that is.

@mdickinson mdickinson merged commit ecf5de5 into main Oct 3, 2022
@mdickinson mdickinson deleted the stubs-fixes branch October 3, 2022 07:32
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