Refactoring on Source Parsing Related Functions

[srilman]

Description

This PR consists of some of the refactoring done in the first 2 commits of #316. However, this PR applies some of the changes that were recommended in the comments of that PR. In total, this PR does the following:

• Move some of the Pydantic models from src_parser/init.py to the new src_parser/models.py
• Move the make_lock_spec and parse_source_files functions from conda_lock.py to src_parser/init.py
• Move the parse_pyproject_toml function to the end of pyproject_toml.py

[netlify]

✅ Deploy Preview for conda-lock ready!

Name	Link
🔨 Latest commit	8e36877
🔍 Latest deploy log	https://app.netlify.com/sites/conda-lock/deploys/63f9400f1625bc00082eeeb6
😎 Deploy Preview	https://deploy-preview-347--conda-lock.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[maresb]

This commit style is superb!!! It's a breeze to review now.

@mariusvniekerk, do you have any objections to moving the location of the models like this? While the main use of conda-lock is that of a CLI, I was thinking that in case someone is instead using conda-lock as a library, this could cause breakage. (In that case we may want to wait for a major release?)

[maresb]

Can't you combine these into a single import statement?

Also, stylistically in most cases I tend to prefer absolute imports since they're more explicit; you don't have to remember which path the current file is in to find the target.

Would it make more sense to have these in conda_lock.models.dependency or similar? It may be confusing to have models in two different places.

[srilman]

Originally, I did have this as a single import statement. But when I ran the pre-commit, Black re-formatted it to multiple imports like so.

[srilman]

Also BTW, by reexporting the models from conda_lock.src_parser.models to conda_lock.src_parser like we do in src_parser/__init__.py, anyone who uses this code like a library can still import these classes from conda_lock.src_parser without anything breaking. That's why I'd prefer to keep conda_lock.src_parser.models rather than conda_lock.models.dependency

[srilman]

I can definitely use absolute imports though

[maresb]

Originally, I did have this as a single import statement. But when I ran the pre-commit, Black re-formatted it to multiple imports like so.

That's really weird. I've never seen Black do that before, and I can't reproduce it. I always get

from .models import (
    Dependency,
    LockSpecification,
    Selectors,
    URLDependency,
    VersionedDependency,
)

Also BTW, by reexporting the models from conda_lock.src_parser.models to conda_lock.src_parser like we do in src_parser/__init__.py, anyone who uses this code like a library can still import these classes from conda_lock.src_parser without anything breaking.

👍

That's why I'd prefer to keep conda_lock.src_parser.models rather than conda_lock.models.dependency

I think I'm missing something here. What goes wrong when using conda_lock.models.dependency? Since conda_lock.src_parser.models doesn't yet exist in any release, nobody is depending on it, so don't we have full freedom in choosing the location?

To explain my motivation, when I glance at the structure, I see conda_lock.models, and that makes me expect that the models are defined there. And then looking at src_parser/__init__.py, I see .models, and my first thought is that it's conda_lock.models rather than conda_lock.src_parser.models. (I'm easily confused. 😂)

In case we do want to move things around in a way that would break backwards compatibility, we could always add deprecation warnings as per the example in PEP 562.

Thanks a lot for all your effort and patience here, it's much appreciated!!! 😄

[srilman]

I'm still confused about your motivation. Are you trying to define some sort of public interface?

No I'm really just trying to avoid moving things around too much. However, its fine, I can move them to conda_lock.models. In fact, that might be the better idea since those models are used at every stage of conda-lock

Although I still think conda_lock.models should be renamed to conda_lock.common, since the conda_lock.models.channel module consists of both models and helper functions.

[maresb]

Hi @srilman, thanks so much for getting back to this!

Is the idea behind putting __all__ in src_parser/__init__.py to retain backwards compatibility? If so, the latest thinking about the next release is to do a v2, so this may be our opportunity to restructure things without worrying about backwards-compatibility. 😄

[maresb]

Ah, I just saw your messages now...

It is essentially just complaining that the imports were not used anywhere.

For this you can also add # noqa: F401 (unused import) to the end of the red line. The (unused import) part is stylistic rather than functional.

No I'm really just trying to avoid moving things around too much. However, its fine, I can move them to conda_lock.models. In fact, that might be the better idea since those models are used at every stage of conda-lock

I see. Let's go with your "better idea" thanks to the major release.

Although I still think conda_lock.models should be renamed to conda_lock.common, since the conda_lock.models.channel module consists of both models and helper functions.

I think I understand why, and it's because you got me thinking about circular imports. If you look at channel.py, and in particular the function

def env_var_normalize(url: str) -> CondaUrl:

this function uses the CondaUrl constructor, thus it depends on the CondaUrl class. On the other hand, CondaUrl.from_string invokes env_var_normalize. Thus the class and helper are circularly dependent in a fairly essential way.

Maybe we could better clarify the situation by adding an underscore-prefix to the functions in conda_lock.models to indicate that they're private helpers?

[srilman]

I'm going to admit, I don't have an opinion for or against this anymore; I just want to get this PR in. So I'm just going to rename src_parser/models.py to models/lock_spec.py

[srilman]

I also got rid of __all__ from src_parser/__init__.py as well.

[maresb]

Sorry @srilman for the arduous process. At least I think we're substantially increasing the code quality with this PR.

I added some commits which I think make this ready. Could you please review them?

I believe I managed to resolve the circular import by moving aggregate_lock_specs to its own module. Now the imports can be loaded at top-level without errors. I also hope I resolved your concern regarding conda_lock.models by marking the helper functions as private (with a _ prefix).

[maresb]

I just rebased on main, and I squashed a few of the commits with minor corrections into the corresponding main commit. Once the tests are green, let's merge without squashing.