Skip to content

Remove optional From Dependency Info in Internal Code - #389

Merged
maresb merged 5 commits into
conda:mainfrom
srilman:remove-optional-flag
May 20, 2023
Merged

Remove optional From Dependency Info in Internal Code#389
maresb merged 5 commits into
conda:mainfrom
srilman:remove-optional-flag

Conversation

@srilman

@srilman srilman commented Mar 13, 2023

Copy link
Copy Markdown
Contributor

This PR removes any references to an optional flag or option from the entire codebase. This builds on top of PR #388, so make sure to get that one merged first. At that point, there is an implicit invariant for all dependencies; the dependency is optional if category != "main". Therefore, storing the optional value just makes the code unnecessarily complex.

Note that this does not affect how source files are parsed or the produced lock file in any way. The step that writes the lockfile out will insert optional: true or optional: false flags depending on the category.

@srilman
srilman requested a review from a team as a code owner March 13, 2023 00:21
@netlify

netlify Bot commented Mar 13, 2023

Copy link
Copy Markdown

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 9203c8d
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/64692edc07fbd00008f6dda4
😎 Deploy Preview https://deploy-preview-389--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 settings.

@srilman
srilman force-pushed the remove-optional-flag branch from b372c6a to 5e96205 Compare March 13, 2023 01:03

@maresb maresb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From a thorough reading of the changes, I'm quite confident that this is a pure refactor. LGTM, and significantly increases code clarity.

If you could rebase on main I'd like to merge. Thanks @srilman!

Comment thread conda_lock/conda_lock.py

for p in lockfile.package:
if p.platform == platform and ((not p.optional) or (p.category in categories)):
if p.platform == platform and p.category in categories:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note to self: not p.optionalp.category == "main".
Since "main" in categories, this is redundant and can be deleted.

Therefore, this is logically sound.

raise ValueError(f"{path} has unknown version {version}")

for p in content["package"]:
del p["optional"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note to self: we need to delete the optional key in order to parse this as a StrictModel.

@maresb

maresb commented May 20, 2023

Copy link
Copy Markdown
Contributor

Or does it make more sense to merge #390 all at once, once I've completed that?

@maresb

maresb commented May 20, 2023

Copy link
Copy Markdown
Contributor

Ah, there are serious merge conflicts with #290 now in main. 😞

It looks like you're both doing similar things, so I wonder if this also fixed the same bugs.

Sorry, I seem to have been confused.

@srilman

srilman commented May 20, 2023

Copy link
Copy Markdown
Contributor Author

@maresb Resolving conflicts now ...

updates:
- [github.com/pre-commit/mirrors-mypy: v1.0.1 → v1.1.1](pre-commit/mirrors-mypy@v1.0.1...v1.1.1)
@srilman
srilman force-pushed the remove-optional-flag branch from d0d86be to 5ee8289 Compare May 20, 2023 20:31
@srilman

srilman commented May 20, 2023

Copy link
Copy Markdown
Contributor Author

Merged with main, everything should be good!

@maresb

maresb commented May 20, 2023

Copy link
Copy Markdown
Contributor

Looking great, thanks!!!

As we wait for the Windows test to finish, can we rebase #390 on this?

Comment on lines +212 to +213
"metadata": json.loads(
content.metadata.json(

@maresb maresb May 20, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just noticed that in #390 you have here instead "metadata": content.metadata.dict( which seems more elegant. Is this an accident, or does this cover some edge case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe there was an issue when encoding content.metadata as a dict due to a special class / container that was used in the metadata (was it a frozenset?). Let me check for certain.

@maresb

maresb commented May 20, 2023

Copy link
Copy Markdown
Contributor

The tests are passing. Let's pick up on #389 (comment) with #390. I have to stop for tonight.

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.

2 participants