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

Introduce dependency sorting #3996

Closed
wants to merge 7 commits into from
Closed

Introduce dependency sorting #3996

wants to merge 7 commits into from

Conversation

br3ndonland
Copy link

@br3ndonland br3ndonland commented Apr 24, 2021

Resolves: #312
Resolves: #4383

  • Added tests for changed code.
  • Updated documentation for changed code.

Description

Thank you for Poetry. It's a huge help to the Python community, and I enjoy working with it.

When installing a package with poetry add, the package is appended to the appropriate section of the pyproject.toml. Some developers have their dependencies sorted alphabetically, so they have to manually sort the new dependency into the appropriate place in pyproject.toml. It would be great to have the dependencies automatically sorted.

This PR will introduce the ability to sort dependencies alphabetically when adding. Dependency sorting can be configured with poetry config dependencies.sort true or export POETRY_DEPENDENCIES_SORT=true. If true, dependencies will be sorted alphabetically when adding. For the default dependency group, python will be at the top of the list.

Related

#312 (comment)
#312 (comment)
#2308
#4260

@dlech
Copy link

dlech commented May 18, 2021

It would be great if this was enabled by default. Or if that is not acceptable, add a setting to poetry.toml so that it can be enforced on a per-project basis rather than having to train everyone to always type in --sorted.

@br3ndonland
Copy link
Author

It would be great if this was enabled by default. Or if that is not acceptable, add a setting to poetry.toml so that it can be enforced on a per-project basis rather than having to train everyone to always type in --sorted.

Thanks for the feedback! I agree, I would like to see sorting become the default behavior.

The poetry.toml config setting is a great idea - maybe something like poetry config dependencies.sort true or export POETRY_DEPENDENCIES_SORT=true.

@br3ndonland
Copy link
Author

The latest commit will remove the --sorted (-S) option. Instead, dependency sorting can be configured with poetry config dependencies.sort true or export POETRY_DEPENDENCIES_SORT=true. If true, dependencies will be sorted alphabetically when adding, keeping python at the top of the list.

The dependencies.sort option will default to false to avoid changing the previous default behavior, but as mentioned in the comments, dependency sorting could potentially be enabled by default in the future.

I'll look forward to some input from the maintainers.

@br3ndonland br3ndonland changed the title command/add: introduce --sorted option command/add: introduce dependency sorting May 21, 2021
donegjookim
donegjookim previously approved these changes Jun 2, 2021
@donegjookim
Copy link

@br3ndonland Good work!

@sonarcloud
Copy link

sonarcloud bot commented Jul 5, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@br3ndonland
Copy link
Author

Dependency sorting has now been updated for Poetry dependency groups (added in #4260 and 1.2.0a2).

@finswimmer finswimmer added area/config Related to configuration management kind/feature Feature requests/implementations labels Mar 29, 2022
@finswimmer finswimmer requested a review from a team March 29, 2022 07:15
Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

In general, looks good. 👍

Just one thing (besides the nitpicks below): I think the sort option should be considered in poetry init, too. Can you add this, please? 🙏

tests/console/commands/test_add.py Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
@br3ndonland br3ndonland changed the title command/add: introduce dependency sorting Introduce dependency sorting Apr 17, 2022
@br3ndonland
Copy link
Author

In general, looks good. 👍

Just one thing (besides the nitpicks below): I think the sort option should be considered in poetry init, too. Can you add this, please? 🙏

Thank you for your review!

I'm happy to update poetry init to support dependency sorting. I've taken a first pass at it with the latest commit. Feel free to make further suggestions if you would like to see it implemented differently.

local_config_file = TOMLFile(Path.cwd() / "poetry.toml")
local_config_file_contents = document()
local_config_file_contents.update({"dependencies": {"sort": True}})
local_config_file.write(local_config_file_contents)
Copy link
Author

Choose a reason for hiding this comment

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

The dependency sorting in poetry init currently works like this:

  • For noninteractive use, add a --sort-dependencies flag.
  • For interactive use, after the user has added all their dependencies, ask them if they want to sort the dependencies alphabetically.
  • If dependency sorting is selected (either interactive or noninteractive), Poetry will write a poetry.toml file to the new project with dependencies.sort set to true.

Happy to consider suggestions on the implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Hey,

in my opinion we shouldn't introduce another flag or questions here and just respect whatever is set in the config.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, that's fine. I reverted the new flag.

By "the config," you mean the user's global Poetry config?

I guess the approach would be to read the global config, and if there's a dependencies.sort setting there, use that.

@@ -91,6 +93,7 @@ def handle(self) -> int:
)
return 1

config = Config()
Copy link
Author

Choose a reason for hiding this comment

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

Updated to read the global config, instead of adding a new flag to init (#3996 (comment))

@abn
Copy link
Member

abn commented Apr 17, 2022

One issue I have with this is that this is a client level configuration for a project level preference.

For example, in poetry, we specify poetry-core and default plugins at top right after python constraint.

Personally, I like an always sorted model, but I don't think this is desirable across the board.

@radoering
Copy link
Member

One issue I have with this is that this is a client level configuration for a project level preference.

Can't we just create a local project specific config.toml and explicitly enable/disable sorting if it's relevant for the project. Or should the project specific config.toml be considered a user specific file, too?

@abn
Copy link
Member

abn commented Apr 17, 2022

Good question. I'm not sure if I have a definitive answer here. While that will work, I don't think we've considered project config.toml something we expect to be checked in. Additionally, I'd be reluctant to suggest folks who don't want this to disable it in yet another project file. And yes, historically this has been something for client side project preferences and not project level.

@br3ndonland
Copy link
Author

Dependency sorting has now been updated for the renamed dependency group (default -> main), introduced in #5465 and 1.2.0b2.

@Kavan72
Copy link

Kavan72 commented Aug 14, 2022

@abn @br3ndonland any update on this?

#3996

When installing a package with `poetry add`, the package is appended to
the appropriate section of the pyproject.toml. Some developers have
dependencies sorted alphabetically, so they have to manually sort the
new dependency into the appropriate place in pyproject.toml.

This commit will add a poetry config setting `dependencies.sort`, which
can also be configured with the corresponding environment variable
`POETRY_DEPENDENCIES_SORT`. If `true`, dependencies will be sorted
alphabetically when adding, keeping `python` at the top of the list.

Initially, the setting was configured with a `--sorted (-S)` option, but
it is easier to have a config setting instead of repeatedly having to
enter the `--sorted` option.

Poetry dependency groups (added in 1.2.0a2) will be included and tested.
#3996 (review)

- For noninteractive use, add a `--sort-dependencies` flag.
- For interactive use, after the user has added all their dependencies,
  ask them if they want to sort the dependencies alphabetically.
- If dependency sorting is selected (interactive or noninteractive),
  Poetry will write a `poetry.toml` file to the new project with
  `dependencies.sort` set to `true`.
#3996 (review)
#3996 (comment)

Dependencies will only be sorted if `dependencies.sort` is `true` in the
user's global Poetry config.
Dependency sorting will be updated for the renamed dependency group
(`default` -> `main`), introduced in #5465 and
1.2.0b2 (https://github.com/python-poetry/poetry/releases/tag/1.2.0b2).
Tests were previously using django-pendulum 0.1.6-preview.4, but this
apparently now points to 0.1.6rc4. This commit will downgrade the
version of django-pendulum in the dependency sorting tests.
@br3ndonland br3ndonland requested a review from radoering August 15, 2022 22:16
@radoering
Copy link
Member

One issue I have with this is that this is a client level configuration for a project level preference.

Unfortunately, I think we need consensus on this. Is it ok to have a client level configuration for this or should it be a project specific option in pyproject.toml. Especially @abn

@radoering radoering added the status/needs-consensus Consensus among maintainers required label Aug 16, 2022
@br3ndonland
Copy link
Author

Sure, happy to discuss further.

One issue I have with this is that this is a client level configuration for a project level preference.

I don't understand the issue. Isn't this just a commentary on Poetry's config system in general? The feature proposed by this PR works in the same way as any other Poetry config setting.

If you want it to be a "project level setting," users would follow the paradigm suggested in the current docs on local configuration:

Your local configuration of Poetry application is stored in the poetry.toml file, which is separate from pyproject.toml.

So users would run poetry config dependencies.sort true --local, and check in the poetry.toml.

@@ -230,6 +231,15 @@ def handle(self) -> int:
)
)

if self.poetry.config.get("dependencies.sort") is True:
sorted_dependencies = OrderedDict(sorted(section.items()))

Choose a reason for hiding this comment

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

One more issue here: casting section to dictionary causes loss of comments in project file. That might be critical for some projects. Please, try to fix the issue

Copy link
Author

Choose a reason for hiding this comment

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

One more issue here: casting section to dictionary causes loss of comments in project file. That might be critical for some projects. Please, try to fix the issue

If comments were preserved, where would they go? We could probably preserve inline comments, but single line comments would be more difficult.

The most common use case for comments that I have seen is breaking up dependencies into subsections, like this:

[tool.poetry.dev-dependencies]
# tests
pytest = "*"
# docs
mkdocs = "*"

If the dependencies were sorted, where would the comments go? It would probably end up like this:

[tool.poetry.dev-dependencies]
# tests
# docs
mkdocs = "*"
pytest = "*"

A more effective approach might be to remove the comments and separate these commented sections into Poetry 1.2 dependency groups.

[tool.poetry.group.test.dependencies]
pytest = "*"

[tool.poetry.group.docs.dependencies]
mkdocs = "*"

I'm happy to try preserving comments if there is a suggested data structure from TOML Kit.

Choose a reason for hiding this comment

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

Also comments might be used to temporarily disable a dependency or explain a dependency directly below the comment.
In these cases, it's quite important to keep those comments. I think it would be good to make the comments stick to the next dependency line:

[tool.poetry.dependencies]
simplejson = "^3.17.5"  # used in requests to serialize decimals
# Consider to replace requests with httpx 
# to improve performance.
# httpx = "^0.23.0"
requests = "^2.26.0"
fastapi = "^1"

should turn into

[tool.poetry.dependencies]
fastapi = "^1"
# Consider to replace requests with httpx 
# to improve performance.
# httpx = "^0.23.0"
requests = "^2.26.0"
simplejson = "^3.17.5"  # used in requests to serialize decimals

Your example is about a case that can be resolved using poetry functional, as you mentioned. Users should be responsible for proper use of Poetry. So I would keep comments in this case as well

Choose a reason for hiding this comment

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

I don't know how to use tomlkit properly. But I can just share my thoughts.

It seems comment lines are stored in section.value.body list with None as a key and Comment(..) as a value. You can't get an access to the comment lines via section.items() method. I didn't find any other way to access to the comments but section.value.body. And I don't see methods to remove comment lines from the section (neither section.remove() nor section.clear() don't remove the comments). So, you should re-create an entire section and append sorted lines into it, or try to sort section.value.body in-place.

@br3ndonland
Copy link
Author

Closing this PR because it has been open for almost two years, the maintainers have still not reached consensus, I am not willing to endlessly maintain this branch, I am tired of dealing with the flaky Cirrus CI jobs, and I am no longer using Poetry (br3ndonland/inboard#56).

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/config Related to configuration management kind/feature Feature requests/implementations status/needs-consensus Consensus among maintainers required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

when adding dependencies, insert them at the correct ordering index in the list sorting packages
8 participants