Skip to content

Better organize lint and test dependencies#636

Merged
webknjaz merged 9 commits intoaio-libs:masterfrom
lysnikolaou:organize-deps
Mar 17, 2025
Merged

Better organize lint and test dependencies#636
webknjaz merged 9 commits intoaio-libs:masterfrom
lysnikolaou:organize-deps

Conversation

@lysnikolaou
Copy link
Copy Markdown
Contributor

@lysnikolaou lysnikolaou commented Mar 12, 2025

What do these changes do?

  • Rename ci deps to test deps to only install test deps in test CI job.
  • Move build, pre-commit, twine and types-setuptools to lint deps
  • Move tox to dev dep since it's never used in CI
  • Move cython into its own file so that it can be used as a constraint
  • Fix CI jobs and Makefile to use the appropriate requirement files

Are there changes in behavior for the user?

No.

Checklist

  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

lysnikolaou and others added 3 commits March 12, 2025 12:09
- Rename ci deps to test deps to only install test deps in test
  CI job.
- Move build, pre-commit, twine and types-setuptools to lint deps
- Move tox to dev dep since it's never used in CI
- Move cython into its own file so that it can be used as a constraint
- Fix CI jobs and Makefile to use the appropriate requirement files
@lysnikolaou
Copy link
Copy Markdown
Contributor Author

Approval needed for CI to run here.

Comment on lines -95 to -96
env:
PIP_CONSTRAINT: requirements/ci.txt
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this removal is okay or not. Since it's a pure python build (thus not installing cython), I don't think this has any effect, but I may be mistaken.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ideally, it should start pinning setuptools too. I just didn't get to it.

Copy link
Copy Markdown
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

I think this is mostly fine as long as CI and RTD don't crash. I'll probably watch to change things up at some point but that's no reason to block.

@webknjaz webknjaz enabled auto-merge (squash) March 13, 2025 16:10
@lysnikolaou
Copy link
Copy Markdown
Contributor Author

It seems that codecov has not come back with a status report, which blocks the auto-merge. Checking https://app.codecov.io/gh/aio-libs/frozenlist/pull/636 manually confirms that there's no coverage change.

@webknjaz
Copy link
Copy Markdown
Member

Oh, it looks like @Dreamsorcerer reduced the number of uploads in #620 for some reason and Codecov is now waiting for a few more, which aren't going to happen. We should recover the MyPy matrix, it seems.

@lysnikolaou
Copy link
Copy Markdown
Contributor Author

Can this PR be merged nevertheless? I can open a follow-up that recovers the mypy matrix if that helps.

@Dreamsorcerer
Copy link
Copy Markdown
Member

As mentioned in another thread, I think the mypy matrix will be unmaintainable, so probably better to just fix the number of expected uploads.

@lysnikolaou
Copy link
Copy Markdown
Contributor Author

lysnikolaou commented Mar 17, 2025

This can probably be merged now. codecov reports that coverage is less than the target coverage, but that's also true for master (actually, coverage is identical both here and on master).

@webknjaz webknjaz merged commit c28f32d into aio-libs:master Mar 17, 2025
41 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants