8000 Deprecate --dev related switches by qmarcou · Pull Request #775 · conda/conda-lock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
/ conda-lock Public

Deprecate --dev related switches #775

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Deprecate --dev related switches #775

wants to merge 8 commits into from

Conversation

qmarcou
Copy link
Contributor
@qmarcou qmarcou commented Feb 18, 2025

Description

This PR is a follow up of #641 .

In a nutshell --dev/--no-dev switches are incorrectly documented, inconsistent, as well as redundant (albeit with some clashes) with the --extra CLI.

Changes made deprecate explicitly all --(no-)dev(-dependencies) by raising a meaningful exception.

Limits:

@qmarcou qmarcou requested a review from a team as a code owner February 18, 2025 14:46
@netlify Netlify
Copy link
netlify bot commented Feb 18, 2025 •
edited
Loading

✅ Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit f753f70
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/67b49d574f0814000842fcdc
😎 Deploy Preview https://deploy-preview-775--conda-lock.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@maresb maresb mentioned this pull request Feb 21, 2025
Open
@maresb
Copy link
Contributor
maresb commented Feb 23, 2025

Hi @qmarcou, thanks so much for all your work on this. Sorry to take so long to respond, this ended up going a lot deeper than I expected.

I want to move things forward with deprecations, but as I've said I'm also concerned about breaking stuff, especially since the upcoming release is unfortunately going to be so big. The current logic involved is beyond my ability to reason about it, so the only way I could get a handle on it was to test all 48 possible combinations of relevant CLI arguments in #778.

I've extended your work a bit, and cherry-picked the tests in qmarcou#2. I would like to know what you think.

@qmarcou
Copy link
Contributor Author
qmarcou commented Mar 7, 2025

Hi @maresb ,
Apologies I have been busy and slacking on this.
So in the end you favored a soft deprecation over the strict one you initially mentioned in #641?

From the user perspective, I think we should strive to make it as easy as possible to migrate from breaking changes. For example, I like the idea of deprecating an argument and failing with an error of how to switch to its direct replacement argument. If someone is maintaining some workflow, and we need to break it, then I'd like for them to have a 5-minute fix.

Thank you so much for your help with setting up CLI tests for all the optional dependencies switches. In the end did it help you figure out the current and desired default behavior for the lock and install commands?

@maresb
Copy link
Contributor
maresb commented Mar 7, 2025

Thanks so much @qmarcou for coming back to this, I really appreciate it!

I think I've decided on a hard deprecation, but with a flag that can be used as an escape hatch to restore the previous behavior. (I have some uncommitted changes where I think I've correctly replicated the previous behavior.)

Regarding the desired defaults, that's beyond my horizon at the moment. I need to focus fully on releasing conda-lock v3 and not breaking stuff. There's a lot in my head about potential failure modes between mamba v1 and v2, and I still need to make some changes to the test suite.

Once v3 is out (and assuming I'm not inundated with bug reports) then we can immediately figure out the desired default behavior. And now that you know my priorities (simple and clear migration path, and preferring "category" terminology to "extras"), you'll probably be able to figure out the correct solution and let me know once I'm ready. 😄

@maresb
Copy link
Contributor
maresb commented Apr 2, 2025

I want to get v3 out tonight, so I'm going to let this slip. Sorry. 😞

I plan to release much more frequently in the future, so I don't expect it to be such a big deal to get this in. I sincerely do value your contribution and feedback.

@qmarcou
Copy link
Contributor Author
qmarcou commented Apr 3, 2025

No worries, and congratulations for getting v3 out!
Apologies I have been quite busy lately, and I wasn't sure whether you were waiting for action on my part before merging qmarcou#2 in this current PR.
I have just unlocked the workflow runs, and I could chnage the UserWarning to FutureWarning and we should be good to go?

@maresb
Copy link
Contributor
maresb commented Apr 3, 2025

Thanks @qmarcou!

Supposing we fix the warning and merge my PR, there are still two things on my mind:

  1. Should we also merge Add regression test for dev dependencies #778?
  2. I think my PR was triggering some spurious deprecation warnings when running without any arguments, so I think my detection for the distinction between omitted flags and explicitly-specified default flags may still be broken.

@qmarcou
Copy link
Contributor Author
qmarcou commented Apr 8, 2025 •
edited
Loading

I think it would be good to also merge #778 , the code is well centralized and easy to remove if you decide to completely drop "--dev-dependency" support one day.

Regarding your second point, I did not have time to look into details, but could the new UserWarning raised by the deprecation be the cause of this discrepancy of the expected number of warnings? Though this should not happen when running without any argument.

I have added some comments to your proposed changes, but I may be a bit lost into the changes and their scope, they may be irrelevant (sorry for wasting your time in that case)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@qmarcou @maresb
0