Skip to content

Conversation

farhannadim311
Copy link

This PR introduces PEP 735 dependency groups (dev, docs, pre-commit, towncrier) to both pyproject.toml and tox.ini.

This ensures contributors can reproduce the exact development, testing, and docs environments with consistent tooling. It replaces ad-hoc dependency lists and centralizes them for clarity. I made sure, the styling of the files stay the same

Solves inconsistencies in developer environments and makes tox easier to run by mapping environments directly to dependency groups.

If merged, contributors can run:

  • pip install .[dev] → dev/test tools
  • pip install .[docs] → docs build
  • tox -e pre-commit → linting hooks
  • tox -e towncrier-check → changelog check

Refs #3634

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@farhannadim311
Copy link
Author

I would love feedback on this, as I want to merge my code, I did no styling changes and as for the ios test case failing, it managed to install all the dependencies, so it doesn't seem like a dependency related issue, however let me know what you guys think. i am willing to change any documentation if needed although i might need some guidance for that

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

This is headed in the right direction; a couple of comments inline about some of the specifics.

However, the biggest question I have is why there haven't been any changes to the existing definitions of optional groups. This PR adds dependency groups - but doesn't remove the existing definitions. Even the PR description says:

If merged, contributors can run:

pip install .[dev] → dev/test tools
pip install .[docs] → docs build
...

which... isn't using dependency groups at all, and in my testing at least, doesn't work as described.

"pre-commit == 4.3.0",
]

"town-crier" = [
Copy link
Member

Choose a reason for hiding this comment

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

Towncrier is a single word:

Suggested change
"town-crier" = [
"towncrier" = [

Comment on lines +111 to +114
# -------------------------------------------------------------------
# PEP 735 dependency groups (root): used by tox & contributors.
# These do NOT ship in built wheels.
# -------------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to document what dependency groups are.

Suggested change
# -------------------------------------------------------------------
# PEP 735 dependency groups (root): used by tox & contributors.
# These do NOT ship in built wheels.
# -------------------------------------------------------------------

Comment on lines +20 to +21
-e{tox_root}{/}core
-e{tox_root}{/}travertino
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason these have been changed to editable installs?

commands =
# TOGA_INSTALL_COMMAND is set to a bash command by the CI workflow
{env:TOGA_INSTALL_COMMAND:python -m pip install {tox_root}{/}core[dev] {tox_root}{/}dummy {tox_root}{/}travertino}
{env:TOGA_INSTALL_COMMAND:python -m pip install -e {tox_root}{/}core -e {tox_root}{/}dummy -e {tox_root}{/}travertino}
Copy link
Member

Choose a reason for hiding this comment

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

Again - is there a reason these have been switched to editable installs?

Also - it's not clear to me how/why this will be installing the dev dependencies for core. The [dev] group usage has been removed, but no equivalent dependency group has been added.

commands =
# TOGA_INSTALL_COMMAND is set to a bash command by the CI workflow
{env:TOGA_INSTALL_COMMAND:python -m pip install {tox_root}{/}core[dev] {tox_root}{/}travertino}
{env:TOGA_INSTALL_COMMAND:python -m pip install -e {tox_root}{/}core -e {tox_root}{/}travertino}
Copy link
Member

Choose a reason for hiding this comment

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

Again - why editable installs?

"sphinx-toolbox == 4.0.0",
"sphinxcontrib-spelling == 8.0.1",
"pyenchant == 3.2.2",
"Pillow == 11.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

It seems a little weird that Pillow is defined twice. We probably want to define a optional group that includes Pillow, and then include optional as a sub-dependency of docs and dev.

@freakboy3742
Copy link
Member

Also - the iOS test failure isn't anything you've done - the iOS simulator sometimes has a bad day for no particular reason. I've kicked off a re-run of that test.

@johnzhou721
Copy link
Contributor

Hmm... I found out that #3719 already included this work... just pointing that out as my 2 cents. I don't know which will take priority though -- in my own opinion it'd depend on the timeframe of it getting merged, but I'm unfamiliar with the new MkDocs stuff.

@freakboy3742
Copy link
Member

Hmm... I found out that #3719 already included this work... just pointing that out as my 2 cents. I don't know which will take priority though -- in my own opinion it'd depend on the timeframe of it getting merged, but I'm unfamiliar with the new MkDocs stuff.

The one that takes priority is the one that is finished first. In this case, there's a number of outstanding review items on this PR.

@freakboy3742
Copy link
Member

Thanks for the contribution here; however, as noted in the comments, this has now been resolved indirectly by #3719.

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.

3 participants