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

Migrate project metadata to PEP 621 #132

Merged
merged 6 commits into from
Mar 23, 2024

Conversation

maxtheman
Copy link

@maxtheman maxtheman commented Mar 12, 2024

Upgrade flit to 3+ and refactor pyproject.toml to accommodate new metadata

@maxtheman
Copy link
Author

I worked on this because I'm having a heck of a time getting pre/post/invariants to work in the linter and thought maybe bumping asteroid would work.. I am evaluating deal for my FT project and was impressed with the hypothesis support (versus icontract)

Here's the issue:
Screenshot 2024-03-12 at 12 56 06 PM
Notice that the lint is only picking up the min.py tests, and the .pre and .inv tests in app.py don't get picked up (but are correct, I think, per the runtime and tests)

https://github.com/maxtheman/contract_programming_test is the repo. Would appreciate any suggestions or a note if this is intended behavior!

@orsinium
Copy link
Member

I used Rye to manage the project

The project provides a Taskfile.yml. Run task all, and it will create venv, install deps, and run all tests, linters, and formatters for you.

added requirements.lock and requirements-dev.lock

The project intentionally doesn't use a lock file to always be automatically executed on the latest versions of all dependencies. The only way a library can control versions of dependencies used by upstream projects is through the dependency constraints.

CI fails now, but it's not your changes, but rather the recent pytest release. Let me fix it.

@orsinium
Copy link
Member

Notice that the lint is only picking up the min.py tests, and the .pre and .inv tests in app.py don't get picked up

Contracts are not guaranteed to be picked up by linter. From the docs:

Linter silently ignores contract if it cannot be executed.

There can be many reasons to skip them: unresolved dependencies, undesired side-effects, or unsupported syntax. Linter and verifier work on "best effort" and favor reliable results over false positives (aka "success typing").

Digging through your commit history, I see that you specify in app.py contracts on methods. Methods cannot be checked by linter (except staticmethods, if I remember correctly) because it would need to resolve not only the method call but also the class instantiation, and all arguments for both the call and the infantilization must be statically inferrable as well.

pyproject.toml Outdated Show resolved Hide resolved
@orsinium
Copy link
Member

I was on vacation past week. Now that I'm back you can expect much faster replies from me. Usually, within a day.

@maxtheman
Copy link
Author

@orsinium Thank you for the thorough review, I learned a bit here.

You're right, with the newer version sphinx docs fail to build on my machine. I was confused about the inclusion of a higher version in the integration features (it's at 4.5> there), but I see now there was a reason for pinning it to this in the docs features specifically.

Adjusting that back, I did have to add in specific requirements for jinja and docutils. Failing to do so with the requirements provided resulted in these two failures:

Screenshots of the built pages using this code follow:
Screenshot 2024-03-22 at 7 13 23 PM
Screenshot 2024-03-22 at 7 13 34 PM

I think, per your comments about asteroid versioning, the rationale for me starting the PR is based on an incorrect premise. It turns out that I had a conflicting version because I'd installed icontracts in the same repo, and they manager versions differently. It had nothing to do with deel Since I had a lockfile and installed them first, that's what caused the issue.

I considered just closing it out, but updating the pyproject.toml to a modern style and bumping asteroid seems somewhat useful, so I incorporated your comments and pushed an update that I hope suffices.

Regarding your comments on the linter:
Understood regarding: "Linter silently ignores contract if it cannot be executed." -> since it was silent, I wasn't sure if I was doing something wrong or not and wanted to chase it down so I understood the project well. Thank you for the reference and additional info.

From a DX perspective, I do wish there was a way for the user to know which ones the linter was skipping, and for what reason. I figured out the limitations around method calls through experimentation, but having it tell me 'Class.method skipped: not statically inferable' or something similar would be usefl.

For my FT project I ended up using hypothesis rule-base state machine testing as an "good enough" solution, but I enjoyed working with this library and will take a look at it again, for sure.

@maxtheman
Copy link
Author

(if the PR is not useful though feel free to close it out! only interested in providing help where I can actually be helpful :-D)

@orsinium
Copy link
Member

Thank you! There are more issues with sphinx, as it turns out. Let me drop it from this PR and look into it separately. I appreciate your effort to migrate the metadata, I'll merge it shortly 👍🏿

@orsinium orsinium changed the title Bump astroid to 3.1 and flit to 3+ Migrate project metadata to PEP 621 Mar 23, 2024
@orsinium orsinium merged commit 2fa9370 into life4:master Mar 23, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants