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

Migrate to Poetry and modernise #349

Merged
merged 6 commits into from
Jul 29, 2023
Merged

Migrate to Poetry and modernise #349

merged 6 commits into from
Jul 29, 2023

Conversation

tombh
Copy link
Collaborator

@tombh tombh commented Jul 1, 2023

Poetry is currently the most popular Python dependency manager. Whilst it is ostensibly a replacement for the requirements.txt convention it also encourages more general best practices such as; dependency lockfiles, oneliner releasing, task running, and so on.

Note that the lockfile isn't used to force locked dependencies in the officially published packages to PyPI. Wheels stilly only use the conventional versions (Eg ~1.0.3, ^2.3.12, >=3.8.1,<4, etc) specified in pyproject.toml. The lockfile will mostly just be used for reproducibility in CI

New

  • Introduces Ruff, a modern Python linter, that in our case replaces our dependencies on flake8 and bandit. This is also a nice community hat tip as Ruff uses Pygls for its LSP: https://github.com/astral-sh/ruff-lsp
  • Introduces Poe for standardised task running. Eg poetry run poe lint
  • CI now has a separate lint job, so tests can pass but lints can fail, giving better visibility to the critical qualities of PRs.
  • 2 new updates (poe and I can't remember the other right now) require that we have a minium Python version of 3.8.1. Python 3.7 is end of life, so I assume that's ok.
  • Linter to check that the Pygls client has been autogenerated to its most up to date state. This means that the Last Modified: section of the autogenerated file is no longer dynamic, it just references the git command to get the last commit date for the file. When the date is dynamically added, it makes it difficult for git diff to detect if actual relevant code has changed.
  • Duplicate CI jobs are skipped, namely jobs are no longer run for push and pull_request events at the same time, only one is chosen. Thanks to https://github.com/fkirc/skip-duplicate-actions
  • CONTRIBUTORS.md is automated
  • Automate releases by making a Github Release
  • CHANGELOG.md autogenerated during Github Release

TODO

Code review checklist (for code reviewer to complete)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • Docstrings have been included and/or updated, as appropriate
  • Standalone docs have been updated accordingly
  • CONTRIBUTORS.md was updated, as appropriate
  • Changelog has been updated, as needed (see CHANGELOG.md)

@tombh tombh force-pushed the tombh/migrate-to-poetry branch 11 times, most recently from f655733 to 8bbcbee Compare July 2, 2023 02:10
@tombh tombh changed the title ci: Migrate to Poetry and modernise Migrate to Poetry and modernise Jul 2, 2023
@tombh tombh force-pushed the tombh/migrate-to-poetry branch 8 times, most recently from 611259e to 92516c8 Compare July 7, 2023 16:46
@tombh tombh requested a review from alcarney July 7, 2023 16:52
@tombh tombh force-pushed the tombh/migrate-to-poetry branch 4 times, most recently from 1243787 to 4433845 Compare July 7, 2023 18:31
scripts/generate_client.py Outdated Show resolved Hide resolved
alcarney
alcarney previously approved these changes Jul 8, 2023
Copy link
Collaborator

@alcarney alcarney left a comment

Choose a reason for hiding this comment

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

This looks great! Excited to see it all in action 😄

While I've come across poetry before I hadn't heard of poe, looks interesting - I assume you chose it due to it's integration with poetry?

@tombh tombh force-pushed the tombh/migrate-to-poetry branch 2 times, most recently from 4cab42a to d207d38 Compare July 8, 2023 18:43
@tombh tombh requested a review from alcarney July 8, 2023 19:17
alcarney
alcarney previously approved these changes Jul 9, 2023
@alcarney
Copy link
Collaborator

alcarney commented Jul 9, 2023

I know we'd talked about waiting to complete LSP 3.17 compliance before making a new release, so what do you think?

That was perhaps me being over-optimistic 😅, happy to go for a release now.

@tombh
Copy link
Collaborator Author

tombh commented Jul 10, 2023

Ok, just waiting for @dgreisen to do those Github Settings chores then.

I tested the release,yml on Super Glass and found a tiny bug, so pushed that same fix here as well, hence the new review request.

@tombh tombh requested a review from alcarney July 10, 2023 22:53
alcarney
alcarney previously approved these changes Jul 11, 2023
@karthiknadig
Copy link
Contributor

2 new updates (poe and I can't remember the other right now) require that we have a minium Python version of 3.8.1. Python 3.7 is end of life, so I assume that's ok.

FYI, on the python tools repos we keep EOL python support for three months, with a note in our releases asking users to switch to new versions of python (for python 3.7 till Sept 27th). This means that if pygls switches to 3.8 as minimum then tools that we maintain will not update to it till end of September.

@tombh
Copy link
Collaborator Author

tombh commented Jul 12, 2023

That seems totally reasonable @karthiknadig, I'm sure I can find a way to bring this PR back down to 3.7 👍

@tombh tombh force-pushed the tombh/migrate-to-poetry branch 2 times, most recently from d644f06 to 28b41f0 Compare July 23, 2023 23:38
@tombh
Copy link
Collaborator Author

tombh commented Jul 23, 2023

Turns out it was really easy to maintain Python 3.7 support, just needed to change poethepoet from 0.20 to 0.19 and sphinx from 6 to 5

So @dgreisen can you add back the 3.7 check requirements please?

Also, even now, with pinned dependencies, we got the ImportError: cannot import name 'Any' from 'typing_extensions' Pyodide error again! So I don't know what's going on there 🤔

Poetry is currently the most popular Python dependency manager. Whilst
it is ostenibly a replacement for the `requirements.txt` convention it is
also encourages more general best practices such as; dependency
lockfiles, releasing, task running, and so on.

This commit also introduces Ruff, a modern Python linter, that in our
case replaces our dependencies on `flake8` and `bandit`.
Also enable a lint that checks to see if the autogenerated client is
uptodate.
Will this fix the typing_extensions/Any error?
@tombh
Copy link
Collaborator Author

tombh commented Jul 29, 2023

Ok, so I've got this back down to Python 3.7. I think we can merge it now.

@tombh tombh requested a review from alcarney July 29, 2023 18:42
@tombh tombh merged commit 476eaf1 into main Jul 29, 2023
21 checks passed
@tombh tombh deleted the tombh/migrate-to-poetry branch July 29, 2023 21:22
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.

None yet

3 participants