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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialize prettytoml ArrayElements as lists #2561

Merged
merged 12 commits into from Jul 13, 2018

Conversation

Projects
4 participants
@JacobHayes
Contributor

JacobHayes commented Jul 11, 2018

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. 馃槄

assert 'cryptography' in p.lockfile['default']
assert 'pyOpenSSL' in p.lockfile['default']
c = p.pipenv('install')
assert c.return_code == 0

This comment has been minimized.

@JacobHayes

JacobHayes Jul 11, 2018

Contributor

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.

@JacobHayes JacobHayes force-pushed the JacobHayes:2504-fix-array-element-serialization branch from ad5d928 to d1c5ec6 Jul 11, 2018

@JacobHayes JacobHayes force-pushed the JacobHayes:2504-fix-array-element-serialization branch from 241bfa9 to 6e05a3e Jul 11, 2018

@JacobHayes

This comment has been minimized.

Contributor

JacobHayes commented Jul 11, 2018

I can't see the output from buildkite and running tests locally with make test fails for other reasons (will try to work through and add the fixes). I'm guessing something in my added test was broken because the buildkite tests passed before I added this test (and reordered/rebased).

If anyone that can see the error could post it back, much appreciated.

@techalchemy

This comment has been minimized.

Member

techalchemy commented Jul 11, 2018

As an FYI make test doesn't do anything as far as I'm aware? If you want to test locally you just need to run

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 pipenv run pytest -v -n auto -k 'test_function_name'

Buildkite output:

[gw4] linux2 -- Python 2.7.15 /root/.local/share/virtualenvs/pipenv-wInVs968-2.7/bin/python2.7
 
PipenvInstance = <class 'tests.integration.conftest._PipenvInstance'>
pypi = <pytest_pypi.serve.Server object at 0x7f0db1ffc590>
 
    @pytest.mark.extras
    @pytest.mark.lock
    @pytest.mark.vcs
    @pytest.mark.needs_internet
    def test_lock_editable_vcs_with_extras_without_install(PipenvInstance, pypi):
        with PipenvInstance(pypi=pypi, chdir=True) as p:
            with open(p.pipfile_path, 'w') as f:
                f.write("""
    [packages]
    requests = {git = "https://github.com/requests/requests.git", editable = true, extras = ["security"]}
                """.strip())
            c = p.pipenv('lock')
>           assert c.return_code == 0
E           AssertionError: assert 1 == 0
E            +  where 1 = <Command 'pipenv lock'>.return_code
 
tests/integration/test_lock.py:362: AssertionError

----------------------------- Captured stdout call -----------------------------
$ pipenv lock
 
Creating a virtualenv for this project鈥
Pipfile: /tmp/pipenv-5pAYbh-project/Pipfile
Using /root/.local/share/virtualenvs/pipenv-wInVs968-2.7/bin/python2.7 (2.7.15) to create virtualenv鈥
Already using interpreter /root/.local/share/virtualenvs/pipenv-wInVs968-2.7/bin/python2.7
Using real prefix '/usr'
New python executable in /tmp/pipenv-5pAYbh-project/.venv/bin/python2.7
Also creating executable in /tmp/pipenv-5pAYbh-project/.venv/bin/python
Installing setuptools, pip, wheel...done.
 
Virtualenv location: /tmp/pipenv-5pAYbh-project/.venv
Locking [dev-packages] dependencies鈥
Locking [packages] dependencies鈥
 
CRITICAL:notpip._internal.index:Could not find a version that satisfies the requirement pyOpenSSL>=0.14 (from requests[security]->-r /tmp/pipenv-ebQyJY-requirements/pipenv-xdUlRl-constraints.txt (line 2)) (from versions: )
Warning: Your dependencies could not be resolved. You likely have a mismatch in your sub-dependencies.
  You can use $ pipenv install --skip-lock to bypass this mechanism, then run $ pipenv graph to inspect the situation.
  Hint: try $ pipenv lock --pre if it is a pre-release dependency.
No matching distribution found for pyOpenSSL>=0.14 (from requests[security]->-r /tmp/pipenv-ebQyJY-requirements/pipenv-xdUlRl-constraints.txt (line 2))
@JacobHayes

This comment has been minimized.

Contributor

JacobHayes commented Jul 11, 2018

@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 TypeError: Object of type 'ArrayElement' is not JSON serializable, then force pulling, and testing again.

@techalchemy

This comment has been minimized.

Member

techalchemy commented Jul 11, 2018

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?

@JacobHayes

This comment has been minimized.

Contributor

JacobHayes commented Jul 11, 2018

Haha yes, it's good on my end. I'll move the relevant details in that comment to #2568.

@techalchemy

This comment has been minimized.

Member

techalchemy commented Jul 11, 2018

Awesome thanks. I put my specific comments there but am super appreciative of your effort and am excited to get those updates merged!

@techalchemy

This comment has been minimized.

Member

techalchemy commented Jul 11, 2018

@frostming or @uranusjr would one of you be able to look this over before we merge? Need someone who knows something about toml

@techalchemy

This comment has been minimized.

Member

techalchemy commented Jul 12, 2018

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 socks?

@JacobHayes

This comment has been minimized.

Contributor

JacobHayes commented Jul 12, 2018

Seems the sub-dep is no longer frozen and is instead stored in the package extras. I forget whether Pipenv is supposed store hashes of sub deps of editable paths, but regardless the fix should be pushed.

@uranusjr

This comment has been minimized.

Member

uranusjr commented Jul 12, 2018

The idea looks correct to me, but I wonder if we should add more types to check, or maybe check against one of the collections.abc types instead.

@frostming

This comment has been minimized.

Collaborator

frostming commented Jul 12, 2018

The fix is OK, though a bit dirty, may be better if ArrayElement inherits list so we don't need to hack json.dumps

@uranusjr uranusjr added this to In progress in 2018.7.x Release via automation Jul 12, 2018

@JacobHayes

This comment has been minimized.

Contributor

JacobHayes commented Jul 12, 2018

Having ArrayElement subclass list would work alright, but similar would also need to be done for AbstractTable (dict-likes) and AtomicElement (int, str, date, etc). The latter wouldn't quite work since it represents multiple distinct python types.

I don't think checking against a collections.abc helps since primitive_value is not a standard attribute (unless we were to try to recreate the primitive_value based on the ABC, which is what primitive_value does for us, but again wouldn't work for AtomicElement). However, that leads us to just checking for hasattr(obj, 'primitive_value'), which should work for any of the mentioned prettytoml types listed above.

JacobHayes and others added some commits Jul 12, 2018

@uranusjr uranusjr force-pushed the JacobHayes:2504-fix-array-element-serialization branch from beae390 to 66964e8 Jul 13, 2018

@uranusjr

This comment has been minimized.

Member

uranusjr commented Jul 13, 2018

@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?

@uranusjr uranusjr force-pushed the JacobHayes:2504-fix-array-element-serialization branch from 66964e8 to 98a5214 Jul 13, 2018

@JacobHayes

This comment has been minimized.

Contributor

JacobHayes commented Jul 13, 2018

@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)?

@uranusjr

This comment has been minimized.

Member

uranusjr commented Jul 13, 2018

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鈥檛 be surprised if it leaks some other container types through.

I鈥檓 adding TokenElement to the isinstance check to cover our bases. That should resolve everything, I hope!

Also allow TokenElement in lock file encoder
Can't be too careful hadling things here.

@uranusjr uranusjr merged commit b9423b6 into pypa:master Jul 13, 2018

1 check passed

buildkite/pipenv Build #893 passed (7 minutes, 29 seconds)
Details

2018.7.x Release automation moved this from In progress to Done Jul 13, 2018

@JacobHayes JacobHayes deleted the JacobHayes:2504-fix-array-element-serialization branch Jul 13, 2018

@techalchemy

This comment has been minimized.

Member

techalchemy commented Jul 14, 2018

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment