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

feat: add support for py3.8 and downgrade some of the types #39

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

nichmor
Copy link

@nichmor nichmor commented Jul 26, 2024

No description provided.

pixi.toml Outdated
ruff = ">=0.5.2,<0.6"
pyyaml = ">=6.0.1,<7"
jsonschema = ">=4.23.0,<5"
pytest = ">=8.3.1,<9"

[tasks]
generate = "python model.py > schema.json"
generate = "python conda_recipe_v2_schema/model.py > new_schema.json"
Copy link
Member

Choose a reason for hiding this comment

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

this should be reverted, right?

Copy link
Author

Choose a reason for hiding this comment

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

yes :)

@wolfv
Copy link
Member

wolfv commented Jul 26, 2024

Good work, definitely uglier :)

@nichmor
Copy link
Author

nichmor commented Jul 26, 2024

Good work, definitely uglier :)

there was a possibility to use https://pypi.org/project/eval-type-backport/ , so we could keep the | union syntax under the pydantic models, but outer variables ( which are not under pydantic class ) should still be Union. In my opinion, it's better to maintain some sort of same stylistic in entire project and do not rely on some implicit behaviour for types under pydantic

@@ -8,14 +8,13 @@ platforms = ["win-64", "linux-64", "osx-64", "osx-arm64"]
conda-recipe-v2-schema = { path = ".", editable = true }

[dependencies]
pydantic = ">=2.8.2,<3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Author

@nichmor nichmor Jul 26, 2024

Choose a reason for hiding this comment

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

as it is a pure python wheel that is published, I thought that it would be better if we keep using PyPI dependencies only when running tests and not get the conda's

Copy link
Contributor

Choose a reason for hiding this comment

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

That is not true :)
Actually we only publish it on conda forge and not on pypi.

Copy link
Author

@nichmor nichmor Jul 26, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes?

But the dependencies still come from conda-forge?

Copy link
Author

Choose a reason for hiding this comment

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

sorry, but I don't get the point here of keeping both pydantic in pyproject.toml and pixi.toml ( by pixi having first-class support of pypi dependencies )

Copy link
Contributor

Choose a reason for hiding this comment

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

If we would drop it from pyproject.toml, we couldn't publish it to pypi if we wanted to.

Until we have a way in pixi to automatically install dependencies declared in pyproject.toml from conda, duplication is the best way as far as I can tell.

Copy link
Author

@nichmor nichmor Jul 26, 2024

Choose a reason for hiding this comment

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

why in this case to not keep it only in pyproject.toml, and use pydantic and any other dependencies from pypi ( which pixi does provides for us and test with them) - this unlock the possibility to publish recipe-format as wheel on pypi.
we still need to duplicate pydantic in conda-forge recipe.yaml, so we have 3 places where we duplicate it ( instead of 2 )

Copy link
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

Still disagree with removing pydantic from the dependencies, but it's not important enough to block on this :)

@baszalmstra
Copy link
Contributor

I also prefer having the conda dependency in there, but also wont block the PR.

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.

4 participants