-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Serialize prettytoml ArrayElements as lists #2561
Serialize prettytoml ArrayElements as lists #2561
Conversation
tests/integration/test_lock.py
Outdated
assert 'cryptography' in p.lockfile['default'] | ||
assert 'pyOpenSSL' in p.lockfile['default'] | ||
c = p.pipenv('install') | ||
assert c.return_code == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is largely a copy of test_lock_editable_vcs_without_install
above, adding the extras
mark, swapping the ref
for extras
in the spec, and adding checks for the security packages.
ad5d928
to
d1c5ec6
Compare
241bfa9
to
6e05a3e
Compare
I can't see the output from buildkite and running tests locally with If anyone that can see the error could post it back, much appreciated. |
As an FYI pip install -e .
pipenv install --dev
pipenv run pytest -v -n auto --ignore pipenv/patched --ignore pipenv/vendor tests To run a specific test you can replace the last command with Buildkite output:
|
@techalchemy Thanks for the testing help and failure output. The test should be fixed now. I've run it locally to verify the test case triggers the intended error (since the code fix was committed before the test fix) by rebasing out the code fix commit, testing (and verifying if fails with |
there are a lot of words here that I'm not really sure are relevant to the PR. Is this good to go in your view? |
Haha yes, it's good on my end. I'll move the relevant details in that comment to #2568. |
Awesome thanks. I put my specific comments there but am super appreciative of your effort and am excited to get those updates merged! |
@frostming or @uranusjr would one of you be able to look this over before we merge? Need someone who knows something about toml |
Ah there is a failure right now also: assert 'pysocks' in p.lockfile['default']
AssertionError: assert 'pysocks' in {'certifi': {'hashes': ['sha256:13e698f54293db9f89122b0581843a782ad0934a4fe0172d2a980ba77fc61bb7', 'sha256:9fa520c1bac... ['socks'], 'git': 'https://github.com/requests/requests.git', 'ref': '4499401a55dedea27831fd9b3c6249b9679f3a2d'}, ...} seems this is now called |
Seems the sub-dep is no longer frozen and is instead stored in the package |
The idea looks correct to me, but I wonder if we should add more types to check, or maybe check against one of the |
The fix is OK, though a bit dirty, may be better if |
Having I don't think checking against a |
….com/JacobHayes/pipenv into 2504-fix-array-element-serialization
beae390
to
66964e8
Compare
@JacobHayes I tried to make the encoding process cleaner with a custom encoder class, and check against a real base class instead of relying on duck-typing. Could you confirm if this solution works for you? |
66964e8
to
98a5214
Compare
@uranusjr Yup, that works for me. As is, it wouldn't cover TokenElements (ex: AtomicElement for str/int/basic types), but it doesn't seem like any of those filter through afaik. I think this sufficently solves the issue, but perhaps another question worth asking is why do ArrayElements filter through the codebase when basic ones do not (ie: is there a literal leaky abstraction somewhere)? |
The locking process does not try very hard to coerce things to built-in types, but relies more on duck-typing to function. If something looks like a sequence, it is happy. I wouldn’t be surprised if it leaks some other container types through. I’m adding |
Can't be too careful hadling things here.
thanks for the cleanup on this and for allowing me to continue to be ignorant in this area of the codebase. This is particularly appreciated! |
Applies the fix suggested in #2504 (comment)
Resolves #2504
I couldn't find a quick/easy way to test this locally before pushing (just saw the Makefile), so using buildkite to test a bit blind. 😅