Skip to content

Rename poe tasks #137

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

Merged
merged 15 commits into from
Jul 14, 2022
Merged

Rename poe tasks #137

merged 15 commits into from
Jul 14, 2022

Conversation

twoertwein
Copy link
Member

This should make poe --help easier to skim and clearer.

image

Happy to also rename Steps (in scripts/test/_step.py) and the github workflow steps, if the changes in pyproject.toml are okay.

script = "scripts.test.run:pyright_src"

[tool.poe.tasks.pytest_src]
help = "CI Test | Run pytest against source code"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we keep this one in there? It's useful sometimes to just run pytest and not mypy and pyright ?

Copy link
Member Author

Choose a reason for hiding this comment

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

test_src has some redundancy with the other tests. Pytest can still be run with poe test_src --profile=pytest.

I think there are two options:

  1. add more options to test_src (for example only mypy and only pyright) and remove pytest_src, mypy_src, and pyright_src or
  2. keep pytest_src but use test_src only for running all local tests (remove mypy+pyright, only pytest, only pre-commit).

I slightly prefer option 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added pytest_src back and removed the profiles from test_src

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about this:

  • have test_src have no profile, do mypy, pyright, pytest and style
  • add back pytest_src
  • add style

then we have the following options:

  • local tests
    • test_src (does all of the next 4)
    • mypy_src
    • pyright_src
    • pytest_src
    • style
  • dist tests (each of the following builds the dist, installs the dist, renames the source, does the test(s), renames the source back, uninstalls the dist)
    • test_dist (does all of the next 3)
    • mypy_dist
    • pyright_dist
    • pytest_dist (should add that)

Don't think we need install_dist and rename_src as top level options.

Copy link
Member Author

Choose a reason for hiding this comment

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

* add `style`

Yeah, that is missing right now.

* dist tests (each of the following builds the dist, installs the dist, renames the source, does the test(s), renames the source back, uninstalls the dist)
  
  * `test_dist` (does all of the next 3)
  * `mypy_dist`
  * `pyright_dist`

I like that as it makes testing the installation easier while developing but it will make the CI slower (unless the CI just calls test_dist)

  * `pytest_dist` (should add that)

tests/ is not part of the wheel (and I think it is good that it isn't part of the wheel)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that as it makes testing the installation easier while developing but it will make the CI slower (unless the CI just calls test_dist)

I'm fine if the CI just calls test_dist or just calls the 3 individual pieces (which I think it does now)

tests/ is not part of the wheel (and I think it is good that it isn't part of the wheel)

If you look at the poe run, both mypy_dist and pyright_dist test the tests folder against the installed distribution, so we're fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now it looks like this:

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks good

@twoertwein twoertwein marked this pull request as ready for review July 13, 2022 22:48
@twoertwein twoertwein requested a review from Dr-Irv July 14, 2022 11:49
Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

Thanks @twoertwein . Nice improvement.

@Dr-Irv Dr-Irv merged commit b5d2e7a into pandas-dev:main Jul 14, 2022
@twoertwein twoertwein deleted the poe branch September 21, 2022 15:27
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.

2 participants