-
Notifications
You must be signed in to change notification settings - Fork 85
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
Make examples directories flake8-clean #1353
Conversation
@@ -37,6 +38,7 @@ def notify_scores_content_change(self, event): | |||
"(Event type: {event.__class__.__name__})".format(event=event) | |||
) | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These examples are being included in the user manual literally. I think I deliberately reduced the newlines so that they don't take up so much space on the doc. That was entirely subjective so please feel free to ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I guessed, but I don't feel bad about having the examples in the doc follow flake8 style. I would like these examples to be included in the style check generally, so the alternative is to add a per-file-ignore for this particular error in the setup.cfg
(adding a # noqa
comment doesn't seem like a good option for something appearing in the docs). It's a tradeoff, but this seems like the most straightforward solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I don't care about the extra white space on the doc that much. I just thought I'd mention. This is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Observed two import errors, but they are existing issues.
@@ -235,7 +235,8 @@ class Apartment(HasTraits): | |||
|
|||
# --<Imports>------------------------------------------------------------------ | |||
|
|||
from traits.api import * | |||
from traits.api import ( | |||
Adapter, adapts, HasTraits, Instance, Interface, Str, Supports) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Orthogonal: On this branch, I get this Import error:
$ python examples/tutorials/traits_4.0/interfaces/
/Users/kchoi/Work/ETS/py39-venv/traits-env/bin/python: can't find '__main__' module in '/Users/kchoi/Work/ETS/traits/examples/tutorials/traits_4.0/interfaces/'
(traits-env) kchoi-mbp2019:traits kchoi$ python examples/tutorials/traits_4.0/interfaces/adaptation.py
Traceback (most recent call last):
File "/Users/kchoi/Work/ETS/traits/examples/tutorials/traits_4.0/interfaces/adaptation.py", line 238, in <module>
from traits.api import (
ImportError: cannot import name 'adapts' from 'traits.api' (/Users/kchoi/Work/ETS/traits/traits/api.py)
On master, I get this:
$ python examples/tutorials/traits_4.0/interfaces/adaptation.py
Traceback (most recent call last):
File "/Users/kchoi/Work/ETS/traits/examples/tutorials/traits_4.0/interfaces/adaptation.py", line 256, in <module>
class PersonINameAdapter(Adapter):
File "/Users/kchoi/Work/ETS/traits/examples/tutorials/traits_4.0/interfaces/adaptation.py", line 259, in PersonINameAdapter
adapts(Person, IName)
NameError: name 'adapts' is not defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks. Indeed we need to either delete or rewrite this particular example file (and similarly for interfaces.py
). I think that's covered by the existing issue #437.
@@ -101,7 +101,7 @@ class Apartment(HasTraits): | |||
the specified class or one of its subclasses. | |||
""" | |||
# --<Imports>------------------------------------------------------------------ | |||
from traits.api import * | |||
from traits.api import HasTraits, Instance, implements, Interface, Str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same orthogonal observation. On master, implements
is not defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added #437 to the milestone for the 6.2 release.
Merged master to resolve conflict with #1358. |
Oops, sorry. I did not notice that. |
This PR builds on #1352 to make all of the examples directories flake8 clean (modulo our current set of flake8 exclusions and ignores).
Includes the commits from #1352. It probably makes sense just to review this PR commit-by-commit, and close #1352.
Closes #756.