Skip to content

Remove code duplication in tutor.py#1034

Merged
mdickinson merged 4 commits into
masterfrom
505-remove-tutor-code-duplication
Apr 28, 2020
Merged

Remove code duplication in tutor.py#1034
mdickinson merged 4 commits into
masterfrom
505-remove-tutor-code-duplication

Conversation

@ievacerny
Copy link
Copy Markdown
Contributor

@ievacerny ievacerny commented Apr 27, 2020

Closes #505

The framework is already implemented in traitsui.extras.demo so there is no need for code duplication here. The framework code is removed and instead tutorial.py makes use of demo code in traitsui.extras.demo.

Checklist

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

@ievacerny ievacerny requested a review from corranwebster April 27, 2020 08:31
@ievacerny ievacerny self-assigned this Apr 27, 2020
def main(root_dir):
# Create a tutor and display the tutorial:
path, name = os.path.splitext(root_dir)
demo(dir_name=root_dir, title='Traits Demos')
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A couple of things I noticed while testing the runner:

  1. Description of doc_examples folder shows <img src="traits_ui_demo.jpg">, which is programmed to be default description if there’s no __init__.py file.
  2. Description of examples folder shows a thrown error:
FileNotFoundError: [Errno 2] No such file or directory: 'doc_examples/examples/__init__.py'

Do we want to add these __init__.py files with descriptions, should this be addressed in traitsui code or is it okay to be like this for now?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Include the __init__.py and add a brief explanation of what is in the directory.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added both __init__.py files but the one indoc_examples isn't shown right now. To show it, use_files=True is needed, but that also picks up doc_examples.rst and with the new tree structure enthought/traitsui#792 bug is exposed. So I think it's better to show a reference to traits_ui_demo.jpg until the bug is fixed.

The explanations I added are very minimal. Didn't want to add the actual contents because that would require manual upkeeping, so I hope this is enough.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@corranwebster Why do we want an __init__.py file in doc_examples? That directory is not a Python package. Is there some part of the machinery in tutor.py that needs this?

Copy link
Copy Markdown
Contributor Author

@ievacerny ievacerny Apr 27, 2020

Choose a reason for hiding this comment

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

tutor.py machinery looks for __init__.py of directories in order to extract their descriptions. It is optional for the root directory, with default description set to <img src="traits_ui_demo.jpg">, but all other subdirectories are expected to have __init__.py or the code raises a non-fatal error.

This would be the only need for __init__.py as far as I know.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Got it. Thanks.

That's not nice, but it is what it is. There's definitely scope for rewriting and improving tutor.py one day. But not today.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 27, 2020

Codecov Report

Merging #1034 into master will increase coverage by 3.32%.
The diff coverage is 97.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1034      +/-   ##
==========================================
+ Coverage   73.05%   76.37%   +3.32%     
==========================================
  Files          51       54       +3     
  Lines        6514     6600      +86     
  Branches     1309     1319      +10     
==========================================
+ Hits         4759     5041     +282     
+ Misses       1363     1211     -152     
+ Partials      392      348      -44     
Impacted Files Coverage Δ
traits/api.py 90.32% <ø> (ø)
traits/trait_base.py 68.83% <ø> (+5.25%) ⬆️
traits/trait_types.py 72.45% <92.59%> (+0.81%) ⬆️
traits/trait_set_object.py 95.42% <97.31%> (+29.20%) ⬆️
traits/trait_list_object.py 97.54% <97.73%> (+23.25%) ⬆️
traits/ctrait.py 71.07% <100.00%> (+12.51%) ⬆️
traits/has_traits.py 72.75% <100.00%> (+1.50%) ⬆️
traits/observers/_i_observer.py 100.00% <100.00%> (ø)
traits/observers/_named_trait_observer.py 100.00% <100.00%> (ø)
traits/observers/_observer_graph.py 100.00% <100.00%> (ø)
... and 19 more

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 d1f72d8...19e9818. Read the comment docs.

Comment thread examples/tutorials/tutor.py Outdated
Copy link
Copy Markdown
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

LGTM!

I ran the code following the documentation instruction and it works fine.
(I did need to manually install configobj which is not a dependency in traits CI environment).

@mdickinson mdickinson merged commit a158d32 into master Apr 28, 2020
@mdickinson mdickinson deleted the 505-remove-tutor-code-duplication branch April 28, 2020 11:43
@ievacerny
Copy link
Copy Markdown
Contributor Author

I ran the code following the documentation instruction and it works fine.
(I did need to manually install configobj which is not a dependency in traits CI environment).

It is a dependency in TraitsUI for demo reasons only (as far as I can tell). Should it be added as a dependency to Traits as well?

@mdickinson
Copy link
Copy Markdown
Member

Yes, let's add it to the development dependencies in etstool.py.

While we're there, we could also add mypy to the main list of dependencies and remove the Python-version-specific hack to only add mypy for Python 3.6.

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.

Tutorial runner code is duplicated in TraitsUI and Traits

5 participants