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

Separate testenv setups for lint and package-checks #216

Merged
merged 13 commits into from
Oct 14, 2023

Conversation

loechel
Copy link
Member

@loechel loechel commented Oct 7, 2023

combined tox testenvs for linting and package checks are not usefull, so seperate those two tests that has been together on a testenv:lint into testenv:package-checks and testenv:lint.

@dataflake
Copy link
Member

We all are aware that Python 3.7 is EOL. But dropping support for it requires a bit more discussion than just declaring it within this tool.

@loechel
Copy link
Member Author

loechel commented Oct 7, 2023

@dataflake I can refactor that change out (Python 3.7 EOL) in a different pull request, as that needs to be done by a certain point. But I do want to have separate testenvs for linting and packaging checks

@dataflake
Copy link
Member

Thank you for factoring out the Python 3.7 parts. I'm ±0 on this PR - you're saying the combined lint environment is not useful, but don't provide an explanation why.

@loechel
Copy link
Member Author

loechel commented Oct 7, 2023

Thanks @dataflake

for me linting and checks if the package is releaseable is something different.
I will run the lint step always before pushing any change to the github repo.
But having unnecessary tests in the linting testenv that fails on every run (--> my case RestrictedPython) is frustrating.
As RestrictedPython does not support future Pythons or even most recent without tests and adoption the lint has always failed.

My own opinion is, that you should actually run all test always before commiting anything. in the long run I would like if we could get pre-commit in the default set of tools. But pre-commit should only test necessary test steps locally. a check of manfifest, or python-versions-check is not necessary locally.

@dataflake
Copy link
Member

I always run all tests myself before committing. I don't mind a test that always fails as long as I know why that is and there will be a fix coming at some point. I don't distinguish between "necessary" and other steps. So in the end I am still ±0 on this change. It seems to be a matter of taste.

@dataflake
Copy link
Member

Actually I just saw a redeeming factor. That new test builds the sdist and tests it. We didn't have that before, I like it.

…thub.com:zopefoundation/meta into meta_separate_setops_for_lint_and_package_checks
@dataflake dataflake merged commit af6bdf3 into master Oct 14, 2023
3 checks passed
@dataflake dataflake deleted the meta_separate_setops_for_lint_and_package_checks branch October 14, 2023 06:28
dataflake added a commit to zopefoundation/Zope that referenced this pull request Oct 14, 2023
dataflake added a commit to zopefoundation/Zope that referenced this pull request Oct 14, 2023
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

Sorry for being late to the party.

@@ -98,7 +98,7 @@ Preparation

The script needs a ``venv`` with some packages installed::

$ python3.11 -m venv .
$ python3 -m venv .
Copy link
Member

Choose a reason for hiding this comment

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

This change is not correct: The code in this repository requires to use Python 3.11+.

Copy link
Member

Choose a reason for hiding this comment

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

You changed that from Python 3.8 to 3.11 in 1ef8b9b but I can't see anything in that change set that requires 3.11+. I know that #209 would require 3.11, but that's not merged.

@@ -271,6 +271,9 @@ updated. Example:
" src/foo/bar.py: E221 E222",
"extend-ignore = D203, W503",
]
additional-plugins = [
"maccabe"
Copy link
Member

Choose a reason for hiding this comment

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

I think you mean: mccabe.

@@ -522,6 +525,10 @@ additional-config
list of strings so the leading white spaces and comments are preserved when
writing the value to ``setup.cfg``.

additional-plugins
Some packages want to have additional flake8 plugins installed.
*Caution:* This option has to be a list of strings.
Copy link
Member

Choose a reason for hiding this comment

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

There is no need for extra *Caution:* as the option name is plural.

@@ -1,6 +1,7 @@
[tox]
minversion = 3.18
envlist =
release-check
Copy link
Member

Choose a reason for hiding this comment

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

I think this env should also run on GHA but it should be allowed to fail without failing the build.
So the release manager is not surprised when running this env before the release that fixes are needed. (I personally rely on GHA being green before the release, I do not always run the tests localy before cutting a release.)

@d-maurer
Copy link

d-maurer commented Oct 17, 2023 via email

dataflake added a commit that referenced this pull request Oct 23, 2023
dataflake added a commit that referenced this pull request Oct 26, 2023
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.

5 participants