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

Poetry #297

Merged
merged 28 commits into from
Feb 5, 2021
Merged

Poetry #297

merged 28 commits into from
Feb 5, 2021

Conversation

vtnate
Copy link
Contributor

@vtnate vtnate commented Jan 29, 2021

Any background context you want to provide?

Our setup process was getting too complicated. Using Poetry lets us create a single venv that everyone uses, while not being a Docker container.
Commit message for 4f84004 has a typo. The word should be Poetry, not python.

What does this PR accomplish?

Removes setup.py and requirements.txt in favor of Poetry's pyproject.toml config file.

Things that need work before this is ready:

  • The test command below fails the teaser_single test, though it may be a flake8 error, I'm not quite sure what the error is telling me.
  • I nuked the docs test in tox.ini because it contains a reference to setup.py which no longer exists with Poetry. Need help to bring that back.
  • We no longer have the package and cmdclass features from the old setup.py. Not sure if we need those or not.
  • I updated the requests package to 2.25.1, after checking that nothing appears to be removed between that update and 2.24.0, our previous version. Double check that this is ok?
  • The pre-commit hook for formatting Modelica files doesn't work. I'd love some help sorting that out.

I haven't found any way for the venv to activate/deactivate automatically as you move in/out of the project dir, a la pyenv-virtualenv. Would be cool if that was a thing.

How should this be manually tested?

poetry install
poetry run tox

What are the relevant tickets?

#299

Screenshots (if appropriate)

@vtnate vtnate added the help wanted Extra attention is needed label Jan 29, 2021
@vtnate vtnate self-assigned this Jan 29, 2021
@macintoshpie macintoshpie self-requested a review February 1, 2021 16:41
Copy link
Contributor

@macintoshpie macintoshpie left a comment

Choose a reason for hiding this comment

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

We no longer have the package and cmdclass features from the old setup.py. Not sure if we need those or not.

I know format_modelica_files is used, but not sure about the outhers. I think cmdclasses will need to be refactored into functions if possible and registered as scripts: https://python-poetry.org/docs/pyproject/#scripts

I updated the requests package to 2.25.1, after checking that nothing appears to be removed between that update and 2.24.0, our previous version. Double check that this is ok?

If the tests pass we should be good. Looks like we're only explicitly using requests

The pre-commit hook for formatting Modelica files doesn't work. I'd love some help sorting that out

I think that probably has to do with the comment above, it needs to be a function

I haven't found any way for the venv to activate/deactivate automatically as you move in/out of the project dir, a la pyenv-virtualenv. Would be cool if that was a thing.

I've never done something like this, but if you wanted to get fancy you could maybe use something like chpwd, detect if directory is a poetry project, then run poetry shell.

tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@vtnate vtnate linked an issue Feb 1, 2021 that may be closed by this pull request
@vtnate
Copy link
Contributor Author

vtnate commented Feb 3, 2021

@macintoshpie I made some more changes that still don't work, specifically the precommit and docs tests from tox.ini.

I'm also not sure if the [tool.poetry.scripts] for schema & license are running. I did confirm the cli works, before my first push.

Can you help me some more with this?

tox.ini Show resolved Hide resolved
@vtnate vtnate marked this pull request as ready for review February 4, 2021 18:21
@vtnate vtnate requested a review from nllong February 4, 2021 18:21
@vtnate vtnate removed the help wanted Extra attention is needed label Feb 4, 2021
@vtnate
Copy link
Contributor Author

vtnate commented Feb 4, 2021

@nllong Are these 3 expected checks a remnant from my changes to the ci config? The checks don't ever run, is that because I did something strange setting up the ci environments?

@nllong
Copy link
Member

nllong commented Feb 4, 2021

@nllong Are these 3 expected checks a remnant from my changes to the ci config? The checks don't ever run, is that because I did something strange setting up the ci environments?

I'm unsure why there are 3 extra checks. I'll review the PR and maybe if there is a change needed it will force a rerun. ┐( ∵ )┌

@macintoshpie
Copy link
Contributor

macintoshpie commented Feb 4, 2021

@nllong @vtnate yeah, now that we use a different test matrix and removed the formatting job it is different. will need to update the repo settings

Copy link
Member

@nllong nllong left a comment

Choose a reason for hiding this comment

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

Looks awesome. I tested this locally and found a few things. Can you review this commit to make sure all is good?

a2d8158

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
poetry.lock Show resolved Hide resolved
tox.ini Show resolved Hide resolved
@vtnate
Copy link
Contributor Author

vtnate commented Feb 5, 2021

@nllong I made a small change to suggest using tox instead of pytest in the readme. Otherwise 👍. Still need to make those 3 hanging tests go away: can you look at the updated ci file for what could be causing those?

pyproject.toml Show resolved Hide resolved
@nllong
Copy link
Member

nllong commented Feb 5, 2021

@nllong I made a small change to suggest using tox instead of pytest in the readme. Otherwise 👍. Still need to make those 3 hanging tests go away: can you look at the updated ci file for what could be causing those?

Cool, just updated the test requirements!

@nllong nllong merged commit 62b088a into develop Feb 5, 2021
@nllong nllong deleted the poetry branch February 5, 2021 20:45
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.

Migrate dependency management to Poetry
3 participants