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

feat: add reverse flag to list command #3705

Merged
merged 17 commits into from
Jan 15, 2025
Merged

Conversation

SandrineP
Copy link
Contributor

@SandrineP SandrineP commented Dec 20, 2024

Adds one of the missing sub-command #3535

@SandrineP SandrineP marked this pull request as ready for review January 7, 2025 10:58
@jjerphan jjerphan added the release::enhancements For enhancements PRs or implementing features label Jan 7, 2025
micromamba/tests/test_list.py Show resolved Hide resolved
libmamba/src/api/list.cpp Outdated Show resolved Hide resolved
@pytest.mark.parametrize("quiet_flag", ["", "-q", "--quiet"])
@pytest.mark.parametrize("env_selector", ["", "name", "prefix"])
@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
def test_list(tmp_home, tmp_root_prefix, tmp_env_name, tmp_xtensor_env, env_selector, quiet_flag):
def test_list(tmp_home, tmp_root_prefix, tmp_env_name, tmp_xtensor_env, env_selector, quiet_flag, reverse_flag):
Copy link
Member

Choose a reason for hiding this comment

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

I know this is maybe beyond this PR because it wasn't done in the first place, but we should test the non --json case as well (you could have missed adding the corresponding std::sort, and no test would have pointed that...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test for the non --json case, but the part checking "conda-forge" is assuming that "res" always has the same format.

Copy link
Member

Choose a reason for hiding this comment

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

This handling of flags might be useful to boil down the two tests into a single one handling all the flags.

flags = filter(None, [json_flag])
output = helpers.run_env("export", "-n", empty_env, *flags)

Copy link
Member

Choose a reason for hiding this comment

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

We discussed that IRL, let's forget about my last suggestion (it would make things more complicated than necessary).

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM beyond a few comments from Hind and this review.

Can you edit the description of the PR to point to the issue you mentioned then? :)

libmamba/src/api/list.cpp Outdated Show resolved Hide resolved
libmamba/src/api/list.cpp Outdated Show resolved Hide resolved
@Klaim
Copy link
Member

Klaim commented Jan 8, 2025

For info, my review is basically Julien's review at this point. :)

libmamba/src/api/list.cpp Outdated Show resolved Hide resolved
micromamba/tests/test_list.py Outdated Show resolved Hide resolved
@pytest.mark.parametrize("quiet_flag", ["", "-q", "--quiet"])
@pytest.mark.parametrize("env_selector", ["", "name", "prefix"])
@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
def test_list(tmp_home, tmp_root_prefix, tmp_env_name, tmp_xtensor_env, env_selector, quiet_flag):
def test_list(tmp_home, tmp_root_prefix, tmp_env_name, tmp_xtensor_env, env_selector, quiet_flag, reverse_flag):
Copy link
Member

Choose a reason for hiding this comment

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

This handling of flags might be useful to boil down the two tests into a single one handling all the flags.

flags = filter(None, [json_flag])
output = helpers.run_env("export", "-n", empty_env, *flags)

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM.

Just suggestions for cosmetic changes.

libmamba/src/api/list.cpp Outdated Show resolved Hide resolved
libmamba/src/api/list.cpp Outdated Show resolved Hide resolved
micromamba/tests/test_list.py Outdated Show resolved Hide resolved
libmamba/src/api/list.cpp Outdated Show resolved Hide resolved
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Final suggestion.

micromamba/tests/test_list.py Show resolved Hide resolved
@jjerphan jjerphan merged commit 630b62f into mamba-org:main Jan 15, 2025
34 checks passed
@SandrineP SandrineP deleted the reverse branch January 15, 2025 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release::enhancements For enhancements PRs or implementing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants