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

fix: fix and refactor --no-lockfile-update #1683

Merged

Conversation

ruben-arts
Copy link
Contributor

@ruben-arts ruben-arts commented Jul 26, 2024

Fixes #1669

Refactors the cli configuration into:

  • ProjectConfig: just the manifest path for now but resusing the cli field
  • PrefixUpdateConfig: All settings related to the update of the prefix and lockfile
  • DependencyConfig: All the settings to specify what type of dependency you want to interact with.

@ruben-arts
Copy link
Contributor Author

Conflicts are for after the weekend when the mind is fresh

@ruben-arts ruben-arts marked this pull request as draft July 26, 2024 15:12
@ruben-arts ruben-arts marked this pull request as ready for review July 29, 2024 08:09
@ruben-arts ruben-arts requested a review from baszalmstra July 29, 2024 08:24
@@ -58,13 +66,13 @@ pub async fn execute(args: Args) -> miette::Result<()> {
// updating prefix after removing from toml
get_up_to_date_prefix(
Copy link
Contributor

@olivier-lacroix olivier-lacroix Jul 29, 2024

Choose a reason for hiding this comment

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

Shouldn't this be entirely skipped when --no-lockfile-update is used? I don't think it's a no-op currently

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise, I am not sure #1669 will actually be fixed?

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 this, but it doesn't really matter for the logic. But just to be sure I can agree ;)

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.

I think we should remove --no-lock-update as a whole because I think its very confusing.

pixi.toml Outdated
@@ -95,3 +95,6 @@ lint = { features = [
schema = { features = [
"schema",
], no-default-feature = true, solve-group = "default" }

[dependencies]
boltons = ">=24.0.0,<25"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, removed it

/// Don't install the environment for pypi solving, only update the lock-file if it can solve without installing.
#[arg(long)]
pub no_install: bool,
pub prefix_update_config: PrefixUpdateConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

This removes the --frozen and --locked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it back

@olivier-lacroix
Copy link
Contributor

I think we should remove --no-lock-update as a whole because I think its very confusing.

@baszalmstra , do you mean the entire concept or the name of the feature?

I feel having the ability to defer lockfile update after a series of operations on the manifest is useful. Pixi solve is fast, but not as fast as no solve at all :-)

@ruben-arts
Copy link
Contributor Author

@olivier-lacroix I think what bas means is that we don't have the --no-lock-update flag as the functionality is the same as doing --no-install --frozen

@olivier-lacroix
Copy link
Contributor

@baszalmstra @ruben-arts , I do not see these are equivalent. But maybe I misunderstand something?

see #1669 (comment)

@ruben-arts
Copy link
Contributor Author

@baszalmstra This PR does not yet remove the --no-lockfile-update but only fixes and refactors the code to behave like the documentation. Would you be okay with merging this before we refactor these flags based on the conclusion of this Discussion: #1707

@ruben-arts ruben-arts enabled auto-merge (squash) August 2, 2024 13:29
@ruben-arts ruben-arts merged commit ae3d521 into prefix-dev:main Aug 2, 2024
24 checks passed
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.

Cannot run remove --no-install
4 participants