Sort packages alphabetically inside each category #5965

daveschaefer · 2023-10-05T03:55:51Z

Add a new [pipenv] directive to sort package names alphabetically inside the category, for easier reading.

sort on install
sort on uninstall
sort on upgrade
Does this need to run on update?

--

Unsure if this is the best place or way to implement. Small prototype to add to discussion in #5964 .

Tests:

before patch:

AssertionError: assert ['atomicwrite...ama', 'build'] == ['atomicwrite...', 'colorama']
  At index 1 diff: 'colorama' != 'build'
  Full diff:
  - ['atomicwrites', 'build', 'colorama']
  ?                  ---------
  + ['atomicwrites', 'colorama', 'build']
  ?                            +++++++++

after patch: pass.

The checklist

Associated issue
A news fragment in the news/ directory to describe this fix with the extension .bugfix.rst, .feature.rst, .behavior.rst, .doc.rst. .vendor.rst. or .trivial.rst (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

When installing any package, sort all package names alphabetically inside the category, for easier reading. Unsure if this is the best place or way to implement. Small prototype to add to discussion in pypa#5964 Tests: before patch: ``` AssertionError: assert ['atomicwrite...ama', 'build'] == ['atomicwrite...', 'colorama'] At index 1 diff: 'colorama' != 'build' Full diff: - ['atomicwrites', 'build', 'colorama'] ? --------- + ['atomicwrites', 'colorama', 'build'] ? +++++++++ ``` after patch: pass.

daveschaefer · 2023-10-05T03:57:41Z

tests/integration/test_install_basic.py

+        assert c.returncode == 0
+        assert "build" in p.pipfile["packages"]
+        assert list(p.pipfile["packages"].keys()) == ["atomicwrites", "build", "colorama"]
+


For this test I just chose three arbitrary packages that are already used inside pipenv's own Pipfile.

matteius · 2023-10-05T09:19:31Z

I would recommend making this a behavior driven by a [pipenv] directive in the Pipfile -- the reason why is because then its opt-in/opt-out at the project level if the directive is in the Pipfile; it will sort the packages alphabetically for the project whenever a change is made, install/uninstall/update/upgrade -- but otherwise it will continue to behave as is for folks that don't want this behavior.

daveschaefer · 2023-10-07T17:00:41Z

I would recommend making this a behavior driven by a [pipenv] directive in the Pipfile -- the reason why is because then its opt-in/opt-out at the project level if the directive is in the Pipfile; it will sort the packages alphabetically for the project whenever a change is made, install/uninstall/update/upgrade -- but otherwise it will continue to behave as is for folks that don't want this behavior.

Hello, thank you for the feedback. Implementing this as a config setting/directive sounds quite reasonable.

The [pipenv] section appears to be the same place that stores settings such as allow_prereleases, disable_pip_input, install_search_all_sources, and keep_outdated. Is that correct?
At first glance, implementing a new setting like this seems like it could simply be an if statement to check for the setting. i.e. in add_pipfile_entry_to_pipfile(), something like:

if self.settings.get("sort_alphabetical"):
    p[category] = dict(sorted(p[category].items())

Does that sound like a good way to do it?

Let me know if there is a particular setting name you want. Perhaps sort_alphabetical?

Sort packages alphabetically inside each category. Currently runs on `install`.

daveschaefer · 2023-10-07T17:11:42Z

Which commands should we make this run on? Perhaps install, uninstall, and upgrade - since those are the commands that normally make changes to the Pipfile?

matteius · 2023-10-08T14:56:54Z

Thanks @daveschaefer -- some thoughts:
Perhaps sort_alphabetical --> sort_pipfile

Which commands should we make this run on? Perhaps install, uninstall, and upgrade

That makes sense, just calling out: upgrade and update (update uses upgrade, but I forget if it touches Pipfile directly so worth taking a look).

`sort_pipfile` , as requested in pypa#5965

using sorting directive

daveschaefer · 2023-10-08T16:02:49Z

Thanks @daveschaefer -- some thoughts: Perhaps sort_alphabetical --> sort_pipfile

Thank you. I have updated the code to use sort_pipfile, and added code and tests for uninstall.

Once this is implemented, is there a place in the docs that I can or should update to mention this new directive?
I don't think I see a specific place in the documentation that discusses the [pipfile] section. Should I add one? I can make that a separate PR if that's best.

daveschaefer · 2023-10-08T16:03:33Z

tests/integration/test_uninstall.py

@@ -243,3 +243,54 @@ def test_uninstall_multiple_categories(pipenv_instance_private_pypi):

        assert "six" not in p.lockfile.get("prereq", {})
        assert "six" not in p.lockfile["default"]
+
+
+@pytest.mark.uninstall


I see some of the uninstall tests are marked with @pytest.mark.install, and some are not. Is this based on whether the test calls install during the test? Something else? Let me know if you want me to mark these two tests with @pytest.mark.install also.

Is this based on whether the test calls install during the test?
Yeah, the test markers needs some separate TLC -- but that is the idea.

Thank you. I have marked these tests with @pytest.mark.install.

I saw there are some other tests in test_uninstall.py that call install, but don't have the mark.
Since I am here editing the file, I marked those tests as well.
I can drop the commit if that's not helpful.

matteius · 2023-10-08T16:05:54Z

I don't think I see a specific place in the documentation that discusses the [pipfile] section. Should I add one? I can make that a separate PR if that's best.

We should include the documentation in this PR -- let's see ... probably this is the closest section that documents the Pipfile: https://pipenv.pypa.io/en/latest/pipfile/

Any TLC you put into the docs would be appreciated.

Based on notes from existing code and release docs Note that `keep_outdated` has been discontinued, so no docs added for it.

daveschaefer · 2023-10-08T16:44:35Z

We should include the documentation in this PR -- let's see ... probably this is the closest section that documents the Pipfile: https://pipenv.pypa.io/en/latest/pipfile/

Any TLC you put into the docs would be appreciated.

Thank you. I added a section to pipfile.md based on the notes I could find from the codebase, docs, and release history.
I also added a note for the sort_pipfile setting that would be created with this patch.
Since it looks like the keep_outdated flag has been discontinued, I left it out and did not write anything for it.

Feedback on any of the docs or wording is welcome.

I may not have time immediately, but I will see if I can add tests and code for sorting on upgrade or update as well.

since the test calls `install` while running. as discussed in pypa#5965

Since the goal of the mark is to note which tests use it.

Currently these fail. Will be fixed shortly in the next patch. Pipfiles can contain different formats for package specifications. Current default behaivour is to sort packages into groups - all string values will be sorted first, followed by all dictionary values. e.g. ``` aa = "*" bb = "*" cc = "*" aaa = {version = "*"} bbb = {version = "*"} ccc = {version = "*"} ``` This will have to be fixed.

This is not as clean as only working with dicts. `p[category]`, the parsed pipfile category, is already of type `tomlkit.items.Table` when it is passed to `_sort_category()`. Currently this is only constructing the table right before data is sent to `write_tom()`.

daveschaefer · 2023-10-11T03:19:45Z

So, the good news: the upgrade command already calls add_pipfile_entry_to_pipfile(), where this code already does the sorting. So we may be able to keep all of the sorting code in one place.

The bad news: sorting categories and passing a dict to write_toml() works when all Pipfile entries have the same value type, but not if the category values have both strings and dictionary values.

What I mean is: If all of the package entries use strings like parse = "*", sphinx = "==4.*", the sorting works fine.
But some package entries use other descriptions, such as a dictionary like gunicorn = {version = "*"}.
In this case the toml sorting and printing will default to sorting the data so that all of the string packages like = "*" are sorted first, as a group, followed by all of the dictionary packages like = {version = "*"} after that.
The Pipfile will look like:

aa = "*"
bb = "*"
cc = "*"
aaa = {version = "*"}
bbb = {version = "*"}
ccc = {version = "*"}

I have added test cases to install and uninstall for this to enforce dealing with it.

Currently I have solved this by implementing a private _sort_category() function in project.py that takes a category dict and builds a tomlkit.table with data in the correct sorted order.
This succeeds, and sorts the Pipfile correctly. But feels like a bit of a kludge.
Technically the p[category] data is already of type tomlkit.table right where _sort_category() is called anyway. It gets called right before the data is written with write_toml(). But it feels less clean.

Let me know what you think. If there is a better way to solve this, I am all ears.
I could also move _sort_category() into toml.py if that's a better place to put the toml doc formatting code and keep it all in one place.

daveschaefer · 2023-10-11T03:20:02Z

Is it helpful if I annotate functions with argument types and return types?

daveschaefer · 2023-10-11T03:22:41Z

tests/integration/test_upgrade.py

@@ -0,0 +1,52 @@
+import pytest


I didn't see any existing integration tests for the upgrade command, so I created this file.
I am attempting to match the pattern of test format from other tests.
Let me know if you want me to put these somewhere else.

matteius · 2023-10-19T10:26:54Z

pipenv/project.py

+    def _sort_category(self, category):
+        # toml tables won't maintain sorted dictionary order
+        # so construct the table in the order that we need
+        sorted_category = dict(sorted(category.items()))


I think this will definitely work, but I am wondering if it would be more efficient to move into the loop and not cast to a dict so like:
for key, value in sorted(category.items():

Correct me if that won't work or there is an issue with it, but thats my last bit of feedback. I'll see if @oz123 can review this too but I want to get this merged into the release for this month.

Thank you very much! That is very kind.

Are there other test cases I should consider or other tests I can add?

I noticed some projects that have # comments on some lines of their Pipfiles. When I add a comment on both string-type lines and dictionary-type lines those are both sorted correctly.

Are there other values or types that can go on a line?

I am wondering if it would be more efficient to move into the loop and not cast to a dict so like: for key, value in sorted(category.items():

Thanks! I created a PR to do that - https://github.com/pypa/pipenv/pull/5990/files

Is it helpful if I create a news fragment .rst ? Or is that no longer useful since the PR has been merged?

the new change is trivial and the docs change you did is good -- we just need to figure out why the docs don't seem to be getting published at some point during the new releases.

For pypa#5964 As suggested in pypa#5965 We don't need to cast to `dict` first, we can just sort.

daveschaefer commented Oct 5, 2023

View reviewed changes

daveschaefer mentioned this pull request Oct 5, 2023

Feature Request: Sort packages alphabetically inside each category #5964

Closed

Added new Pipenv option sort_alphabetical

8e18903

Sort packages alphabetically inside each category. Currently runs on `install`.

daveschaefer added 2 commits October 8, 2023 09:46

Rename sorting directive

9bc4ffa

`sort_pipfile` , as requested in pypa#5965

Implement pipfile sorting for uninstall

7b33b04

using sorting directive

daveschaefer commented Oct 8, 2023

View reviewed changes

daveschaefer added 2 commits October 8, 2023 10:32

Add docs for [pipenv] directives

773fcb5

Based on notes from existing code and release docs Note that `keep_outdated` has been discontinued, so no docs added for it.

Add doc for new sorting directive

f2b7b3a

mark uninstall tests with install

4de4d20

since the test calls `install` while running. as discussed in pypa#5965

matteius added the hacktoberfest-accepted label Oct 8, 2023

daveschaefer added 5 commits October 8, 2023 10:52

Mark install for other tests that install

4010f5d

Since the goal of the mark is to note which tests use it.

Move common sorting code to function

6a5cd68

Add upgrade tests

4802425

daveschaefer commented Oct 11, 2023

View reviewed changes

matteius approved these changes Oct 19, 2023

View reviewed changes

matteius reviewed Oct 19, 2023

View reviewed changes

matteius merged commit 8fd6dfc into pypa:main Oct 20, 2023
16 checks passed

daveschaefer added a commit to daveschaefer/pipenv that referenced this pull request Oct 20, 2023

Simplify sorting code

1a8c9de

For pypa#5964 As suggested in pypa#5965 We don't need to cast to `dict` first, we can just sort.

daveschaefer mentioned this pull request Oct 20, 2023

Simplify sorting code #5990

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort packages alphabetically inside each category #5965

Sort packages alphabetically inside each category #5965

daveschaefer commented Oct 5, 2023 •

edited

daveschaefer Oct 5, 2023

matteius commented Oct 5, 2023

daveschaefer commented Oct 7, 2023

daveschaefer commented Oct 7, 2023

matteius commented Oct 8, 2023

daveschaefer commented Oct 8, 2023

daveschaefer Oct 8, 2023

matteius Oct 8, 2023

daveschaefer Oct 8, 2023

matteius commented Oct 8, 2023

daveschaefer commented Oct 8, 2023

daveschaefer commented Oct 11, 2023

daveschaefer commented Oct 11, 2023

daveschaefer Oct 11, 2023

matteius Oct 19, 2023

daveschaefer Oct 20, 2023

daveschaefer Oct 20, 2023

matteius Oct 21, 2023

Sort packages alphabetically inside each category #5965

Sort packages alphabetically inside each category #5965

Conversation

daveschaefer commented Oct 5, 2023 • edited

The checklist

Choose a reason for hiding this comment

matteius commented Oct 5, 2023

daveschaefer commented Oct 7, 2023

daveschaefer commented Oct 7, 2023

matteius commented Oct 8, 2023

daveschaefer commented Oct 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matteius commented Oct 8, 2023

daveschaefer commented Oct 8, 2023

daveschaefer commented Oct 11, 2023

daveschaefer commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveschaefer commented Oct 5, 2023 •

edited