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

Validation using pure python #57

Merged
merged 14 commits into from
Apr 24, 2024
Merged

Validation using pure python #57

merged 14 commits into from
Apr 24, 2024

Conversation

oz123
Copy link
Contributor

@oz123 oz123 commented Apr 19, 2024

Validation using dataclasses sounded like a great idea, and while I learned a lot from this experiment, I unfortunately released a broken version.
Despite all tests were passing, some public interfaces which were not tested caused severe errors.

Earlier versions of plette saved the original snippets parsed by tomlkit as _data attribute for each instance.
These containers behave like dicts, but actually aren't. Hence, trying to model them as dictionaries and cast away the
original data proved tricky.

This refactor keeps the original approach of keeping the tomlkit containers in _data. In addition, validation is now always enforced, as it does not depend on Cerberus.

I believe this approach is simpler and easier to maintain. The real test though is if I can integrate this into pipenv without changing pipenv's tests or it's usage of plette.

@oz123 oz123 requested a review from matteius April 19, 2024 08:10
@oz123 oz123 force-pushed the validation-using-pure-python branch from 24442fb to c6b3bf3 Compare April 19, 2024 08:18
oz123 added 13 commits April 19, 2024 10:28
Signed-off-by: Oz Tiram <oz.tiram@gmail.com>
Signed-off-by: Oz Tiram <oz.tiram@gmail.com>
Signed-off-by: Oz Tiram <oz.tiram@gmail.com>
Signed-off-by: Oz Tiram <oz.tiram@gmail.com>
Signed-off-by: Oz Tiram <oz.tiram@gmail.com>
Signed-off-by: Oz Tiram <oz.tiram@gmail.com>
Signed-off-by: Oz Tiram <oz.tiram@gmail.com>
Signed-off-by: Oz Tiram <oz.tiram@gmail.com>
Signed-off-by: Oz Tiram <oz.tiram@gmail.com>
Signed-off-by: Oz Tiram <oz.tiram@gmail.com>
Signed-off-by: Oz Tiram <oz.tiram@gmail.com>
@oz123 oz123 force-pushed the validation-using-pure-python branch from c6b3bf3 to e03c20b Compare April 19, 2024 08:42
@oz123
Copy link
Contributor Author

oz123 commented Apr 19, 2024

Note, I intend to release this version as 2.0.0 and mark 1.0.0 as obsolete. There are no "new features" here, but it's again a complete rewrite.

Pipenv's integration tests reveal that plette's test suite
was a bit too simplistic.

tomlkit parsed the following package spec:

zipp = 1.11

as a tomlkit.items.Float, which causes the validation to break.
This commit handles this and add unit and integration test case
for this.

Signed-off-by: Oz Tiram <oz.tiram@gmail.com>
import pytest
from plette import models

def test_requires_python_version():
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if we should have more tests around the Requires model -- noticed this is getting removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems as it is getting removed, because on that branch I split the tests from one file to many small ones.
You can find these tests here:

def test_requires_python_version_no_full_version():

And yes, I agree with you with need more tests! The current suite is very weak, and I have been working constantly to improve it (first, I did add all the examples/Pipfiles and the integration tests around them. This PR also adds another tests for a case which was caught in upstream merging.

I will add more in the near future.

Copy link
Contributor

@matteius matteius left a comment

Choose a reason for hiding this comment

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

Looks like this was a fair bit of work -- would like to see it get into pipenv main because I have some more work to do there before pipenv 3000 and I'd be testing your changes first hand for a while once this lands in main.

@oz123 oz123 merged commit 5ef7fe6 into master Apr 24, 2024
5 checks passed
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.

3 participants