Skip to content

Update dependencies #198

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Mar 18, 2024
Merged

Update dependencies #198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ce0e665
Update CI versions
eemeli Mar 10, 2024
9e25854
Move flake8 config to top level, add a separate CI action for linting
eemeli Mar 10, 2024
27ffc98
Update to flake8 v7, add a dev-requirements.txt file
eemeli Mar 10, 2024
666a18b
Update to isort v5, move its config to top-level pyproject.toml
eemeli Mar 10, 2024
aeeab52
Move mypy config to top-level pyproject.toml
eemeli Mar 10, 2024
ee99286
Drop custom test runner, include tox in dev-requirements.txt
eemeli Mar 10, 2024
087b988
Add black 24 to dev-requirements.txt, update flake8 config to match
eemeli Mar 10, 2024
c44f460
Merge fluent.integration and lint actions into a single ci.yml
eemeli Mar 10, 2024
9943867
Merge/refactor fluent.syntax.yml and fluent.runtime.yml actions into …
eemeli Mar 10, 2024
4244d5e
Add current available Python & PyPY versions to CI tests
eemeli Mar 10, 2024
fe70b64
Bump some requirements in docs/
eemeli Mar 10, 2024
5661358
Apply suggestions from code review
eemeli Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .flake8
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[flake8]
exclude=.tox
extend-ignore = E203
max-line-length=120
61 changes: 61 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
name: CI
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like there are many python versions in use across the different jobs. If this is intentionally, adding a comment specifying an issue link or reason why a particular version is required will make someone (maybe you) reviewing this yaml file in 6 years very appreciative :)

If otherwise you don't need different versions you could pin the version using an environment variable and specifying this across the different jobs. This is just a suggestion.

env:
  - PYTHON_VERSION: "3.10"

jobs:
  lint:
    ...
    with:
      python-version: ${{ env.PYTHON_VERSION }}

Even in a scenario where you need specific versions per purpose, using a variable can also help encapsulate that purpose.

env:
   PYTHON_VERSION_ WITH_SPECIFIC_FEATURE: "3.6"

Copy link
Member Author

Choose a reason for hiding this comment

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

Much of that range is due to inheriting a spread from before. The 3.12 for lint is just the current version and could probably be dropped to rely on the default provided by ubuntu-latest; baseline uses 3.6 because we still claim compatibility with that, and documentation's 3.9 is slightly arbitrary, but I'm loathe to spend many cycles on it given the doc build's complexity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You explained it excellently just now. I Just wonder if pulling that information out of this comment thread into the action definition would be helpful for someone like myself reading the code for the first time without context. Just a light suggestion to do with what you think is best.


on:
push:
branches:
- main
pull_request:

jobs:
lint:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
cache: pip
cache-dependency-path: |
dev-requirements.txt
fluent.syntax/setup.py
fluent.runtime/setup.py
- run: python -m pip install -r dev-requirements.txt
- run: python -m pip install ./fluent.syntax ./fluent.runtime
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess one consequence of merging the linting jobs on the GHA level is that if there is an error installing fluent.syntax you will not know if there would have also been an error in fluent.runtime.

Previously being in separate workflows would have revealed this as they could run separately and in parallel.

Does this have a performance impact on the CI run time as well? Have you considered a reusable action for setting up the environment that can then be shared across many jobs to allow centralizing configuration while still parallelizing execution?

Copy link
Member Author

Choose a reason for hiding this comment

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

Eh, sure, but at the cost of additional complexity and interdependency. I'm trying to keep these actions simple, and tbh I'm not too worried about one error potentially masking a later error, given that it'll be revealed after fixing the first one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be super peaved if I spent a bunch of time fixing one error only to reveal another that could have been known before, but I understand there are tradeoffs. Just wondering if you thought about it.

- run: python -m flake8
- run: python -m mypy fluent.syntax/fluent fluent.runtime/fluent
test:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: [3.7, 3.8, 3.9, "3.10", 3.11, 3.12, pypy3.9, pypy3.10]
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
cache: pip
cache-dependency-path: |
fluent.syntax/setup.py
fluent.runtime/setup.py
- run: python -m pip install ./fluent.syntax ./fluent.runtime
- run: python -m unittest discover -s fluent.syntax
- run: python -m unittest discover -s fluent.runtime

# Test compatibility with the oldest Python version we claim to support,
# and for fluent.runtime's compatibility with a range of fluent.syntax versions.
compatibility:
runs-on: ubuntu-20.04 # https://github.com/actions/setup-python/issues/544
strategy:
matrix:
fluent-syntax:
- ./fluent.syntax
- fluent.syntax==0.19.0
- fluent.syntax==0.18.1 six
- fluent.syntax==0.17.0 six
steps:
- uses: actions/checkout@v3
- uses: actions/setup-python@v4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use setup-python@v5 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Dunno, didn't try. I intend to re-refactor this soon after landing this PR, by incrementing the minimum to around 3.9, which will let us get rid of this ubuntu-20.04 runner as well. The action versions will get updated at that time as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

with:
python-version: 3.6
- run: python -m pip install ${{ matrix.fluent-syntax }}
- run: python -m pip install ./fluent.runtime
- run: python -m unittest discover -s fluent.runtime
18 changes: 7 additions & 11 deletions .github/workflows/documentation.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,18 @@ jobs:
name: build
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4
with:
fetch-depth: 0
- uses: actions/setup-python@v4
- uses: actions/setup-python@v5
with:
python-version: 3.7
python-version: 3.9
- name: Install dependencies
run: |
pip install -r docs/requirements.txt
pip install ./fluent.docs
- name: sphinx-build
run: |
./scripts/build-docs python-fluent
- name: artifact
uses: actions/upload-artifact@v3
- run: ./scripts/build-docs python-fluent
- uses: actions/upload-artifact@v4
with:
name: html
path: |
Expand All @@ -52,9 +49,8 @@ jobs:
needs: [build]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Download artifact
uses: actions/download-artifact@v3
- uses: actions/checkout@v4
- uses: actions/download-artifact@v4
with:
name: html
path: _build
Expand Down
48 changes: 0 additions & 48 deletions .github/workflows/fluent.integration.yml
View file
Open in desktop

This file was deleted.

73 changes: 0 additions & 73 deletions .github/workflows/fluent.runtime.yml
View file
Open in desktop

This file was deleted.

63 changes: 0 additions & 63 deletions .github/workflows/fluent.syntax.yml
View file
Open in desktop

This file was deleted.

7 changes: 7 additions & 0 deletions dev-requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
black ~= 24.0
flake8 ~= 7.0
isort ~= 5.0
mypy ~= 1.0
tox ~= 4.0
types-babel
types-pytz
3 changes: 3 additions & 0 deletions docs/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,16 @@
# Add any paths that contain templates here, relative to this directory.
templates_path = ['_templates']


# Add src_dir/docs/_templates in a hook as we only have the src_dir then.
def setup(app):
app.connect('config-inited', add_templates)


def add_templates(app, config):
config.templates_path.insert(0, f'{app.srcdir}/_templates')


# List of patterns, relative to source directory, that match files and
# directories to ignore when looking for source files.
# This pattern also affects html_static_path and html_extra_path.
Expand Down
Loading