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.

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

Run check-manifest and other linters via GitHub Actions. #396

Merged
merged 12 commits into from
Aug 17, 2022
+25 −36
Merged

Run check-manifest and other linters via GitHub Actions. #396

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
85f861a
Run all linters as part of the CI.
adiroiban Jul 16, 2022
bd4ce62
Update contributing to latest tox lint environments.
adiroiban Jul 16, 2022
6c5b16c
Remove tox env that are not used.
adiroiban Jul 16, 2022
6087488
Add newfragment.
adiroiban Jul 16, 2022
d07f208
Move flake8 config outside of tox.
adiroiban Jul 16, 2022
b983603
Apply suggestions from code review
adiroiban Jul 16, 2022
4d490f7
Revert build tox command. Use separate jobs in GHA.
adiroiban Jul 16, 2022
797967a
Merge branch 'trunk' into 292-more-ci-checks
adiroiban Jul 16, 2022
523b173
Merge branch 'trunk' into 292-more-ci-checks
adiroiban Jul 17, 2022
10cecdb
Update .github/workflows/ci.yml
adiroiban Jul 17, 2022
3721e33
Run flake8 on pre-commit with any python version.
adiroiban Jul 17, 2022
1bb6221
Merge branch 'trunk' into 292-more-ci-checks
adiroiban Aug 17, 2022
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
39 changes: 14 additions & 25 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,47 +151,36 @@ jobs:
run: |
codecov -n "GitHub Actions - ${{ matrix.task.name}} - ${{ matrix.os.name }} ${{ matrix.python.name }}"

check:
name: ${{ matrix.task.name}} - ${{ matrix.python.name }}
lint:
name: Linters
runs-on: ubuntu-latest
needs:
- build
strategy:
fail-fast: false
matrix:
python:
# Using second most recent minor release for whatever little
# increase in stability over using the latest minor.
- name: CPython 3.9
tox: py39
action: 3.9
task:
- name: Check Newsfragment
tox: check-newsfragment
env:
TOX_PARALLEL_NO_SPINNER: 1

steps:
- uses: actions/checkout@v3
with:
fetch-depth: 0

- name: Download package files
uses: actions/download-artifact@v3
with:
name: dist
path: dist/

- name: Set up ${{ matrix.python.name }}
- name: Set up python
uses: actions/setup-python@v3
with:
python-version: ${{ matrix.python.action }}
python-version: "3.10"
Copy link
Member

Choose a reason for hiding this comment

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

These days I tend to keep my checks one version behind (3.9 not 3.10). They are there to check the code, not see if the tools work with the latest Python. But definitely not a big deal, just sharing a thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will revert. thanks. no big preference.
Just trying to reduce the number of redirections :)


- name: Install dependencies
run: python -m pip install --upgrade pip tox

- uses: twisted/python-info-action@v1

- name: Tox
run: tox -c tox.ini -e ${{ matrix.task.tox }}
run: tox -c tox.ini --parallel -e pre-commit

- name: Tox
run: tox -c tox.ini --parallel -e check-newsfragment

- name: Tox
run: tox -c tox.ini --parallel -e check-manifest
Copy link
Member

Choose a reason for hiding this comment

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

Use the relevant names for the step name. Why parallel for single environments? How about if: always() so you get all results in a single run rather than having to iterate through multiple failures via multiple commits and ci runs.



pypi-publish:
# https://github.community/t/is-it-possible-to-require-all-github-actions-tasks-to-pass-without-enumerating-them/117957/4?u=graingert
Expand Down
27 changes: 14 additions & 13 deletions CONTRIBUTING.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Want to contribute to this project? Great! We'd love to hear from you!

As a developer and user, you probably have some questions about our
project and how to contribute.
In this article we try to answer these and give you some recommendations.
In this article, we try to answer these and give you some recommendations.


Ways to communicate and contribute
Expand All @@ -28,10 +28,11 @@ There are several options to contribute to this project:
If you found a bug or have a new cool feature, describe your findings.
Try to be as descriptive as possible to help us understand your issue.

* Check out the Freenode ``#twisted-dev`` IRC channel.
* Check out the Libera ``#twisted`` IRC channel or `Twisted Gitter <https://gitter.im/twisted/twisted>`_.

If you prefer to discuss some topics personally, you may find the IRC
channel interesting.
If you prefer to discuss some topics personally,
you may find the IRC or Gitter channels interesting.
They are bridged.

* Modify the code.

Expand Down Expand Up @@ -68,12 +69,14 @@ We recommend the following workflow:
*Running the test suite* for details.


c. Document any user facing changes in one of the `/docs/` files.
c. Document any user-facing changes in one of the `/docs/` files.
adiroiban marked this conversation as resolved.
Show resolved Hide resolved

d. Create a newsfragment in ``src/towncrier/newsfragments/`` describing the changes and containing information that are of interest for end-users.
d. Create a newsfragment in ``src/towncrier/newsfragments/`` describing the changes and containing information that is of interest to end-users.

e. Create a `pull request`_.
Describe in the pull request what you did and why. If you have open questions, ask.
Describe in the pull request what you did and why.
If you have open questions, ask.
(optional) Allow team members to edit the code on your PR.

#. Wait for feedback. If you receive any comments, address these.

Expand All @@ -98,7 +101,7 @@ The following list contains some ways how to run the test suite:
You may want to add the ``--skip-missing-interpreters`` option to avoid errors
when a specific Python interpreter version couldn't be found.

* To get a complete list about the available targets, run::
* To get a complete list of the available targets, run::

$ tox -av

Expand All @@ -108,15 +111,13 @@ The following list contains some ways how to run the test suite:
$ tox -- towncrier.test.test_project.InvocationTests.test_version

* To run some quality checks before you create the pull request,
we recommend to use this call::
we recommend using this call::

$ tox -e check-manifest,check-newsfragment
$ tox -e pre-commit,check-manifest,check-newsfragment

* Install `pre-commit <https://pre-commit.com/>`_ and activate::
* Or enable `pre-commit` as a git hook::

$ pip install pre-commit
$ pre-commit
Or run as git hook
$ pre-commit install

* To investigate and debug errors, use the ``trial`` command like this::
Expand Down
3 changes: 3 additions & 0 deletions setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[flake8]
# Allow for longer test strings. Code is formatted to 88 columns by Black.
max-line-length = 99
Copy link
Member

Choose a reason for hiding this comment

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

I'm still in the boat of using the program's default config file. Even when using the 'single config file for everything' option you still end up with multiple single config files.

Copy link
Member Author

Choose a reason for hiding this comment

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

The plan it to move more things from setup.py into setup.cfg ... I am using it in #398

Copy link
Member

Choose a reason for hiding this comment

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

Any project using black will still have to have a pyproject.toml for any config there, afaik. Anyways, not a bug, just a passing opinion.

Empty file added src/towncrier/newsfragments/292.misc.rst
View file
Open in desktop
Empty file.
35 changes: 1 addition & 34 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[tox]
envlist = cov-erase, pre-commit, {pypy37,pypy38,py37,py38,py39,py310}-tests, flake8, check-manifest, check-newsfragment, cov-report
envlist = cov-erase, pre-commit, {pypy37,pypy38,py37,py38,py39,py310}-tests, check-manifest, check-newsfragment
isolated_build=true
skip_missing_envs = true

Expand Down Expand Up @@ -33,37 +33,4 @@ commands =
# specifying the entry point script in `{envbindir}`.
coverage run -p --module twisted.trial {posargs:towncrier}
coverage combine -a

[testenv:build]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this going away?

Copy link
Member Author

Choose a reason for hiding this comment

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

my bad. I thought that we don't need it and that it was already moved to GitHub CI only.

I will revert...

My hope was that we can use standard tools without the need of extra tools.

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure where you were going so maybe it isn't needed now? It was an honest question.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is still need.
The towncrier is built once.
Then the wheel is uploaded as an artifact.
Then the tests will download the artifact and run the tests agains it.

I am not convinced that we need all this dance... but I think is better to refactor this in another PR.

allowlist_externals =
bash
changedir = {envtmpdir}
deps =
build
check-manifest>=0.44
twine
setenv =
toxinidir={toxinidir}
skip_install = true
commands =
# could be brought inside tox.ini after https://github.com/tox-dev/tox/issues/1571
bash {toxinidir}/tox_build.sh

[testenv:cov-report]
deps =
coverage
skip_install = true
commands =
coverage html
coverage report

[testenv:cov-erase]
deps =
coverage
skip_install = true
commands =
coverage erase

[flake8]
# Allow for longer test strings. Code is formatted to 88 columns by Black.
max-line-length = 99