Switch to tomlkit for parsing and writing #3191

frostming · 2018-11-08T02:27:14Z

Thank you for contributing to Pipenv!

The issue

Incomplete implementation of #3177

The fix

Remove dump_dict method.
Reorder pipfile only when outline table is present.
Preserve inline table for toml.
Drop prettytoml/contoml from vendors.

The checklist

Associated issue
A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

frostming · 2018-11-08T02:29:27Z

Maybe we can drop prettytoml/contoml from the vendors directory at this point?

techalchemy · 2018-11-08T02:41:00Z

That would be very nice, those libraries are so annoying

techalchemy · 2018-11-08T02:55:54Z

Nice failure...

2018-11-08T02:40:09.9468075Z E           assert 'requests = { version = "*" }' in 'requests = {version = "*"}\nclick = "*"\n'

There are spaces between the {} and the contents in LHS, but not in the RHS...

frostming · 2018-11-08T02:59:00Z

@techalchemy :( toml keeps them but tomlkit suppresses them.

frostming · 2018-11-08T03:44:57Z

All CI passed! 🎆

techalchemy · 2018-11-08T03:46:43Z

I cannot believe this is passing. 🎉

techalchemy · 2018-11-08T05:31:08Z

I want to make sure we aren't using this in Pipfile or some hidden dependency somewhere... did you check this already?

tests/unit/test_vendor.py

pipenv/project.py

techalchemy

awesome, looks good to me so whenever @uranusjr confirms I am guessing we can merge this, thanks for your efforts on the toml patches and cleanup -- this is going to make life a lot easier

frostming · 2018-11-08T10:45:41Z

I found a weird bug handling comments. Please don't merge until I fix this. Thanks

techalchemy · 2018-11-08T18:19:15Z

Marking as WIP for now

techalchemy · 2018-11-08T18:19:29Z

what is the weird handling?

frostming · 2018-11-09T01:28:52Z

Seems a bug of tomlkit, trying to figure out how to update vendors and patch scripts.

I have filed bugs in upstream python-poetry/tomlkit#29 python-poetry/tomlkit#30 if anyone is interested in.

frostming · 2018-11-09T03:38:41Z

@techalchemy @uranusjr Can you guys have another look on the latest changes and we are good to go.

uranusjr · 2018-11-09T04:40:17Z

tests/integration/test_project.py

+[packages.requests]
+version = "*"
+            """.strip()
+            f.write(contents)


Not entirely related to this PR, but it just occurred to me that we need a test to make sure we are reading Pipfile and Pipfile.lock with the correct encoding. It is likely best to have an abstraction around this in Project too (something like with project.open_pipfile('w') as f:).

We had this in Project -- did it go away?

tasks/vendoring/patches/vendor/tomlkit-dump-inline-table.patch

pipenv/project.py

pipenv/vendor/tomlkit/container.py

frostming · 2018-11-10T04:51:41Z

Thanks for the quick response from tomlkit and I just updated tomlkit to 0.5.2 and cleaned some patching stuff.

techalchemy · 2018-11-10T05:40:11Z

Vendoring updates are somewhat complicated, I often take my local copy of pipenv, clone the entire thing to /tmp, re-vendor the relevant package, commit that version, then re-generate patches:

export PIPENV_DIR=$(cwd)
cd /tmp
git clone "$PIPENV_DIR" pipenv
cd pipenv/pipenv/vendor
pip install -t . --no-deps --upgrade <pkgname>==<pkgversion>
rm -rf *.dist-info
git checkout -- <pkgname>.LICENSE <pkgdir>/LICENSE # depends on which got deleted
git add <pkgname>...
git commit -m "Temp commit for updating patches"
cp -R "${PIPENV_DIR}/pipenv/vendor/<pkgname>/files-you-want" pipenv/vendor/<pkgname>/
git diff -p pipenv/vendor/<pkgname> > "${PIPENV_DIR}/tasks/vendoring/patches/vendor/pkgname-whatever.patch"

I've literally never documented that before so we should probably record that...

frostming · 2018-11-10T06:54:03Z

@techalchemy Yeah, it really took me much time to figure it out. I managed to do it by invoke run.

uranusjr · 2018-11-10T07:31:01Z

pipenv/vendor/tomlkit/items.py

+try:
+    from enum import Enum
+except ImportError:
+    from pipenv.vendor.backports.enum import Enum


This uses try-catch, but there’s a lru_cache block that uses if-else. I would recommend unifying the style (probably to try-catch; it seems to be recommended).

lru_cache is got from the original code, I will change to if to adapt the style.

the if/else approach is actually there on purpose, and shoudln't be removed blindly...

The decisions I made about where to use try/except vs if/else aren't stylistic, they have real consequences. We vendor a large number of critical items and we also use a number of runtime hacks that affect sys.path. try/except is not an acceptable approach if you specifically need functionality provided by a backport or a vendored package in situations where something might actually succeed if you import it. Things that come to mind:

pathlib (there is a garbage python 2 backport of a package called pathlib, if you try/except this you will wrongly allow imports of the wrong pathlib which is not api compliant with the standard library

TemporaryDirectory -- Not all backports use weakrefs, this will literally break everything about pipenv

I've actually seen people importing things across python versions, across site packages/user site/local environment and having completely out of date versions from actually the wrong version of python 3

scandir in the vendored format is modified to excluded binaries

tl;dr don't mess with things unless you know the consequences of it...

in most cases, like the Enum case, it doesn't matter at all

techalchemy · 2018-11-11T04:16:52Z

if and when @uranusjr is ok with this I can verify the patches and we can merge

techalchemy · 2018-11-13T12:36:31Z

I want to verify that this and #3196 will play nicely together before merging either of them so that’s the next step

…tenance/merge-3191-3196-3209 - Merge #3191, #3196, and #3209 - Closes #3191 - Closes #3196 - Closes #3214 - Closes #3209 Signed-off-by: Dan Ryan <dan@danryan.co>

Merge #3191, #3196 and #3209

Improve toml parsing

3eba19d

Drops prettytoml/contoml form vendors

e147dc3

clear references in patched.txt

b27d6a7

frostming changed the title ~~Improve toml parsing~~ Switch to tomlkit for parsing and writing Nov 8, 2018

techalchemy requested a review from uranusjr November 8, 2018 05:30

techalchemy added the Type: Vendored Dependencies This issue affects vendored dependencies within pipenv. label Nov 8, 2018

uranusjr reviewed Nov 8, 2018

View reviewed changes

tests/unit/test_vendor.py Outdated Show resolved Hide resolved

frostming added 2 commits November 8, 2018 14:59

Add back some tests

1555200

python 2.7 unicode

53b073c

uranusjr reviewed Nov 8, 2018

View reviewed changes

pipenv/project.py Outdated Show resolved Hide resolved

make tomlkit dump toml's inline table

b3aa66b

frostming force-pushed the improve-toml-parse branch from 506f1d9 to b3aa66b Compare November 8, 2018 09:35

techalchemy approved these changes Nov 8, 2018

View reviewed changes

techalchemy and others added 2 commits November 8, 2018 04:37

Merge branch 'master' into improve-toml-parse

9743d98

Force inline table

65d7090

frostming force-pushed the improve-toml-parse branch from 1b807fa to 65d7090 Compare November 8, 2018 10:18

techalchemy added the DO NOT MERGE label Nov 8, 2018

frostming changed the title ~~Switch to tomlkit for parsing and writing~~ [WIP]Switch to tomlkit for parsing and writing Nov 9, 2018

Final fix

0762528

fix patch

3490fc8

frostming changed the title ~~[WIP]Switch to tomlkit for parsing and writing~~ Switch to tomlkit for parsing and writing Nov 9, 2018

uranusjr reviewed Nov 9, 2018

View reviewed changes

tasks/vendoring/patches/vendor/tomlkit-dump-inline-table.patch Outdated Show resolved Hide resolved

uranusjr reviewed Nov 9, 2018

View reviewed changes

pipenv/project.py Outdated Show resolved Hide resolved

uranusjr reviewed Nov 9, 2018

View reviewed changes

pipenv/vendor/tomlkit/container.py Show resolved Hide resolved

frostming added 3 commits November 9, 2018 12:59

Only convert outline tables when write toml

de78c1e

Move to utils function

accd0ea

update tomlkit

6df7d88

update patch

1f7c9ef

frostming force-pushed the improve-toml-parse branch from 88ad74c to 1f7c9ef Compare November 10, 2018 05:19

backports import

de98b87

uranusjr reviewed Nov 10, 2018

View reviewed changes

Change fallback style

f5c7c58

frostming removed the DO NOT MERGE label Nov 13, 2018

techalchemy mentioned this pull request Nov 13, 2018

Merge #3191, #3196 and #3209 #3217

Merged

techalchemy closed this in #3217 Nov 14, 2018

techalchemy added a commit that referenced this pull request Nov 14, 2018

Merge pull request #3217 from pypa/maintenance/merge-3191-3196-3209

d9ebad3

Merge #3191, #3196 and #3209

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to tomlkit for parsing and writing #3191

Switch to tomlkit for parsing and writing #3191

frostming commented Nov 8, 2018 •

edited

Loading

frostming commented Nov 8, 2018

techalchemy commented Nov 8, 2018

techalchemy commented Nov 8, 2018

frostming commented Nov 8, 2018

frostming commented Nov 8, 2018

techalchemy commented Nov 8, 2018

techalchemy commented Nov 8, 2018

techalchemy left a comment

frostming commented Nov 8, 2018

techalchemy commented Nov 8, 2018

techalchemy commented Nov 8, 2018

frostming commented Nov 9, 2018 •

edited

Loading

frostming commented Nov 9, 2018

uranusjr Nov 9, 2018

techalchemy Nov 10, 2018

frostming commented Nov 10, 2018

techalchemy commented Nov 10, 2018

frostming commented Nov 10, 2018

uranusjr Nov 10, 2018

frostming Nov 10, 2018

techalchemy Nov 10, 2018

techalchemy Nov 10, 2018

techalchemy commented Nov 11, 2018

techalchemy commented Nov 13, 2018

Switch to tomlkit for parsing and writing #3191

Switch to tomlkit for parsing and writing #3191

Conversation

frostming commented Nov 8, 2018 • edited Loading

The issue

The fix

The checklist

frostming commented Nov 8, 2018

techalchemy commented Nov 8, 2018

techalchemy commented Nov 8, 2018

frostming commented Nov 8, 2018

frostming commented Nov 8, 2018

techalchemy commented Nov 8, 2018

techalchemy commented Nov 8, 2018

techalchemy left a comment

Choose a reason for hiding this comment

frostming commented Nov 8, 2018

techalchemy commented Nov 8, 2018

techalchemy commented Nov 8, 2018

frostming commented Nov 9, 2018 • edited Loading

frostming commented Nov 9, 2018

uranusjr Nov 9, 2018

Choose a reason for hiding this comment

techalchemy Nov 10, 2018

Choose a reason for hiding this comment

frostming commented Nov 10, 2018

techalchemy commented Nov 10, 2018

frostming commented Nov 10, 2018

uranusjr Nov 10, 2018

Choose a reason for hiding this comment

frostming Nov 10, 2018

Choose a reason for hiding this comment

techalchemy Nov 10, 2018

Choose a reason for hiding this comment

techalchemy Nov 10, 2018

Choose a reason for hiding this comment

techalchemy commented Nov 11, 2018

techalchemy commented Nov 13, 2018

frostming commented Nov 8, 2018 •

edited

Loading

frostming commented Nov 9, 2018 •

edited

Loading