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

Add pixi list command #665

Merged
merged 27 commits into from
Jan 16, 2024
Merged

Add pixi list command #665

merged 27 commits into from
Jan 16, 2024

Conversation

hadim
Copy link
Contributor

@hadim hadim commented Jan 12, 2024

Original ticket: #238

@hadim
Copy link
Contributor Author

hadim commented Jan 12, 2024

It would be nice to differentiate packages specified in pixi.toml (explicit) from the implicit ones with a different color. What do you think? What would be the best way to achieve this?

@wolfv
Copy link
Member

wolfv commented Jan 12, 2024

@hadim if you can get the specs from the pixi.toml, then it should be as easy as comparing the names of the packages, right? In order to highlight them.

@wolfv
Copy link
Member

wolfv commented Jan 12, 2024

Also: awesome! Thanks :)

@hadim
Copy link
Contributor Author

hadim commented Jan 12, 2024

@hadim if you can get the specs from the pixi.toml, then it should be as easy as comparing the names of the packages, right? In order to highlight them.

I found a way. It's working fine but would need another pair of eyes since .dependencies(Some(SpecType::Run), Some(Platform::current())); is harcoded.

@wolfv
Copy link
Member

wolfv commented Jan 12, 2024

I think you might want to take them as CLI arguments? Including the "environment" (once we enable that feature, so you can ignore that for now).

But something like pixi list --platform osx-arm64 could also be nice.

@hadim
Copy link
Contributor Author

hadim commented Jan 12, 2024

Hum, actually the current logic fetches the installed packages from the current prefix. So it would need a different logic if we want to also display for other platforms.

It raises the point what we want this command to be: the current logic was to stick to what is currently installed in the current prefix.

Here is what I use to get the installed packages:

    let prefix_records = prefix
        .find_installed_packages(None)
        .await
        .map_err(|_| miette::miette!("Cannot find installed packages"))?;

Question: do you want to generalize the command and allow listing for any available platforms? If yes, can you point me to the correct logic to replace the snippet above?

@hadim
Copy link
Contributor Author

hadim commented Jan 12, 2024

@wolfv generalizing to any project's platforms and envs seems best. I will refactor the logic to rely on the lock_file module instead.

src/cli/list.rs Outdated Show resolved Hide resolved
@hadim
Copy link
Contributor Author

hadim commented Jan 12, 2024

Current API:

$ pr cargo run -- list --help
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
     Running `target/debug/pixi list --help`
List project's packages. Highlighted packages are explicit dependencies

Usage: pixi list [OPTIONS] [REGEX]

Arguments:
  [REGEX]  List only packages matching a regular expression

Options:
      --platform <PLATFORM>            The platform to list packages for. Defaults to the current platform
      --json                           Whether to output in json format
      --json-pretty                    Whether to output in pretty json format
      --no-sort                        Whether to sort the package list by name
      --manifest-path <MANIFEST_PATH>  The path to 'pixi.toml'
  -v, --verbose...                     Increase logging verbosity
  -q, --quiet...                       Decrease logging verbosity
      --color <COLOR>                  Whether the log needs to be colored [default: auto] [possible values: always,
                                       never, auto]
  -h, --help                           Print help

src/cli/list.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

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

Thank you @hadim really awesome PR!

No big changes required from my point of view! Just wrap it up and we'll ship it! :shipit:

src/cli/list.rs Outdated Show resolved Hide resolved
src/cli/list.rs Outdated Show resolved Hide resolved
src/cli/list.rs Outdated Show resolved Hide resolved
src/cli/list.rs Show resolved Hide resolved
src/cli/list.rs Outdated Show resolved Hide resolved
@wolfv
Copy link
Member

wolfv commented Jan 15, 2024

@hadim we're using comfy_table in rattler-build. Since we're already using it I am not opposed to it.

src/cli/list.rs Outdated Show resolved Hide resolved
src/cli/list.rs Outdated Show resolved Hide resolved
@hadim hadim requested review from ruben-arts and tdejager January 15, 2024 17:21
Copy link
Contributor

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

Nice PR! I left some small comments.

src/cli/list.rs Outdated Show resolved Hide resolved
src/cli/list.rs Outdated Show resolved Hide resolved
src/cli/list.rs Outdated Show resolved Hide resolved
@hadim
Copy link
Contributor Author

hadim commented Jan 16, 2024

All good on my side here.

Let me know if you catch something else.

Thanks for the reviews everyone!

Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

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

Love it, thanks @hadim

@ruben-arts ruben-arts merged commit 9dc8352 into prefix-dev:main Jan 16, 2024
11 checks passed
@hadim hadim deleted the list branch January 16, 2024 14:55
@pavelzw
Copy link
Contributor

pavelzw commented Jan 26, 2024

Looks awesome! Looking forward to the next pixi release. Thanks @hadim

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.

6 participants