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

Use pyproject.toml instead of setup.py/setup.cfg #2540

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Oct 3, 2024

Description

Use pyproject.toml instead of setup.py/setup.cfg, as this is the modern way of doing things.

Also added some improvements suggested by https://learn.scientific-python.org/development/guides/repo-review/?repo=esmvalgroup%2Fesmvalcore&branch=pyproject-toml

Link to documentation: https://esmvaltool--2540.org.readthedocs.build/projects/ESMValCore/en/2540/contributing.html

Related to ESMValGroup/ESMValTool#3584


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@bouweandela bouweandela added the installation Installation problem label Oct 3, 2024
Copy link

codecov bot commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.67%. Comparing base (a3557ec) to head (1a7c867).

Files with missing lines Patch % Lines
esmvalcore/_task.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2540      +/-   ##
==========================================
- Coverage   94.91%   94.67%   -0.25%     
==========================================
  Files         251      251              
  Lines       14268    14266       -2     
==========================================
- Hits        13543    13506      -37     
- Misses        725      760      +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

absolutely YES for this 🍺

pragma: no cover
if __name__ == .__main__.:
if TYPE_CHECKING:

[pycodestyle]
Copy link
Contributor

Choose a reason for hiding this comment

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

love the outcome of this issue, that was proposing support for pyproject.toml 🤣 PyCQA/pycodestyle#813

Copy link
Member Author

Choose a reason for hiding this comment

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

We should be able to drop pycodestyle as soon as we're using ruff for linting too.

@bouweandela bouweandela marked this pull request as ready for review October 3, 2024 12:39
Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Great, thanks Bouwe, I really like this! 🚀

There's one more suggestion at https://learn.scientific-python.org/development/guides/repo-review/?repo=esmvalgroup%2Fesmvalcore&branch=pyproject-toml about pyproject:

[PP309](https://learn.scientific-python.org/development/guides/pytest#PP309): Filter warnings specified

filterwarnings must be set (probably to at least ["error"]). Python will hide important warnings otherwise, like deprecations.

[tool.pytest.ini_options]
filterwarnings = ["error"]

Would it make sense to implement this as well?

@@ -1,5 +1,3 @@
^\.circleci/
^environment\.yml$
^pyproject.toml$
^setup\.py$
^setup\.cfg$
Copy link
Contributor

Choose a reason for hiding this comment

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

setup.cfg still exists, did you remove that here on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it now only contains the configuration for pycodestyle, so it does not affect the (development) installation of the tool anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Would it make sense to remove all unused element in setup.cfg here? It looks like it contains more than the pycodestyle config:

ESMValCore/setup.cfg

Lines 1 to 38 in a3557ec

[tool:pytest]
addopts =
--doctest-modules
--ignore=esmvalcore/cmor/tables/
--cov-report=xml:test-reports/coverage.xml
--cov-report=html:test-reports/coverage_html
--html=test-reports/report.html
env =
MPLBACKEND = Agg
log_level = WARNING
markers =
installation: Test requires installation of dependencies
use_sample_data: Run functional tests using real data
[coverage:run]
parallel = true
source = esmvalcore
[coverage:report]
exclude_lines =
pragma: no cover
if __name__ == .__main__.:
if TYPE_CHECKING:
[pycodestyle]
# ignore rules that conflict with ruff formatter
# E203: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#slices
# E501: https://docs.astral.sh/ruff/formatter/#conflicting-lint-rules
# W503: https://pycodestyle.pycqa.org/en/latest/intro.html#error-codes
ignore = E203,E501,W503
[pydocstyle]
convention = numpy
[mypy]
# see mypy.readthedocs.io/en/stable/command_line.html
python_version = 3.12
ignore_missing_imports = True
files = esmvalcore, tests

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like I messed up the last merge with main. Should be fixed now.

Copy link
Member Author

Choose a reason for hiding this comment

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

And then I forgot to move the changes from #2456 to pyproject.toml. Should be really fixed now 🤦

@@ -61,7 +61,6 @@ def test_get_cl_fix():
)


@pytest.mark.sequential
@pytest.mark.skipif(
sys.version_info < (3, 7, 6), reason="requires python3.7.6 or newer"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you dropping the sequential mark? Also, would it make sense to remove the following skipif since we're not supporting Python 3.7 anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was removed in #1819, but these entries were forgotten. Using

"--strict-markers",

makes using undefined markers an error.

Copy link
Member Author

@bouweandela bouweandela Oct 15, 2024

Choose a reason for hiding this comment

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

Also, would it make sense to remove the following skipif since we're not supporting Python 3.7 anymore?

Removed in e70ffd2

@bouweandela
Copy link
Member Author

Would it make sense to implement this as well?

I tried, but it causes the tests to fail because some of our dependencies are using deprecated features. It would be good to enable that though, but it will require extra work.

@bouweandela
Copy link
Member Author

bouweandela commented Oct 15, 2024

Would it make sense to implement this as well?

I tried, but it causes the tests to fail because some of our dependencies are using deprecated features. It would be good to enable that though, but it will require extra work.

Created #2555 as a reminder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installation Installation problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants