Skip to content

Conversation

@chamshop
Copy link
Collaborator

Added the etstool.py file and modified it for the tutorial. Only the install function is guaranteed to work currently. Will continue work on this for all setup.py files and tests next.
Also altered the README.md to reflect changes in environment creation.

Added the etstool.py file and modified it for the tutorial. Only the `install` function is guaranteed to work currently. Will continue work on this for all `setup.py` files next.
Also altered the `README.md` to reflect changes in environment creation.
@chamshop chamshop self-assigned this Jun 24, 2022
@chamshop chamshop changed the title Added etstool.py (only install works currently) Added etstool.py (only install and flake8 work) Jun 27, 2022
@chamshop
Copy link
Collaborator Author

Added flake8 functionality.

@jonathanrocher
Copy link
Owner

Thanks @chamshop . Is this ready for review or not yet?

Copy link
Collaborator

@siddhantwahal siddhantwahal left a comment

Choose a reason for hiding this comment

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

I think it'll be nice if tutorial attendees could also use these commands. That way, everyone gets a standardized environment and we mitigate the risk of someone forgetting or mistyping commands.

"matplotlib",
"pandas",
"pillow",
"pyqt5",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should explicitly declare pyface as a dependency, since we directly import from it. We're currently pulling pyface implicitly, probably through traitsui.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will add.

ci/__main__.py Outdated
"edm run -e {environment} -- pip install pyside6<6.2.2")
else:
commands.append("edm run -e {environment} -- pip install pyside6")
elif toolkit == 'wx':
Copy link
Collaborator

Choose a reason for hiding this comment

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

My vote would be to not support the wx backend.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will remove.

ci/__main__.py Outdated

# pip install pyqt5, pyqt6, pyside2 and pyside6, because we don't have them
# in EDM yet
if toolkit == 'pyside2':
Copy link
Collaborator

Choose a reason for hiding this comment

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

pyside2 is now available in EDM, or at least version 5.14.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, will remove this special consideration.

ci/__main__.py Outdated
Comment on lines 264 to 275
elif toolkit == 'pyqt6':
commands.append("edm run -e {environment} -- pip install pyqt6")
elif toolkit == 'pyside6':
# On Linux and macOS, some versions of PySide6 between 6.2.2 and 6.3.0
# are unimportable on Python 3.6 and Python 3.7. See
# https://bugreports.qt.io/browse/PYSIDE-1797. It may be possible to
# remove this workaround once PySide6 6.3.1 or later is released.
if sys.platform in {'darwin', 'linux'}:
commands.append(
"edm run -e {environment} -- pip install pyside6<6.2.2")
else:
commands.append("edm run -e {environment} -- pip install pyside6")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still not sure about why we would need this logic. If we expect tutorial attendees to use the CI module, then allowing them to specify the toolkit can be a useful escape hatch if pyqt5 doesn't work for them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @siddhantwahal , I'm not sure I understand your meaning here...

ci/__main__.py Outdated
click.echo("Creating environment '{environment}'".format(**parameters))
execute(commands, parameters)

if source:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need this if block either as we don't want to install any packages from source.

ci/__main__.py Outdated
help="Install main package in 'editable' mode? [default: --not-editable]",
)
@click.option('--source/--no-source', default=False)
def install(runtime, toolkit, environment, editable, source):
Copy link
Collaborator

Choose a reason for hiding this comment

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

When run as is, this command will install the ets_tutorial package, and not any of the application ones. It'll be useful to document this fact. And perhaps "setup" or "build" would be a better name for this command.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great idea. Will change to "setup".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually going to change it to "build", to avoid any confusion with setuptools "setup".

@chamshop
Copy link
Collaborator Author

chamshop commented Jul 7, 2022

@jonathanrocher This should be ready as an environment builder and flake8 runner. Need to add the ability to run setup.py for all the different app folders next.

@jonathanrocher
Copy link
Owner

This is great @chamshop . ci install and ci flake8 are all we need. Don't worry about setup.pys since that was for unit testing. Let's use the rest of the time for packaging with pyinstaller.

@chamshop chamshop merged commit 5673271 into master Jul 7, 2022
@chamshop chamshop deleted the initial_etstool_install_only branch July 7, 2022 13:04
@chamshop chamshop linked an issue Jul 7, 2022 that may be closed by this pull request
3 tasks
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.

Add unit test and CI infrastructure

4 participants