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

Switch to tomlkit for parsing and writing #3191

Closed
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@frostming
Collaborator

frostming commented Nov 8, 2018

Thank you for contributing to Pipenv!

The issue

Incomplete implementation of #3177

The fix

  1. Remove dump_dict method.
  2. Reorder pipfile only when outline table is present.
  3. Preserve inline table for toml.
  4. 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

This comment has been minimized.

Collaborator

frostming commented Nov 8, 2018

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

@techalchemy

This comment has been minimized.

Member

techalchemy commented Nov 8, 2018

That would be very nice, those libraries are so annoying

@techalchemy

This comment has been minimized.

Member

techalchemy commented Nov 8, 2018

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

This comment has been minimized.

Collaborator

frostming commented Nov 8, 2018

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

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

@frostming

This comment has been minimized.

Collaborator

frostming commented Nov 8, 2018

All CI passed! 🎆

@techalchemy

This comment has been minimized.

Member

techalchemy commented Nov 8, 2018

I cannot believe this is passing. 🎉

@techalchemy

This comment has been minimized.

Member

techalchemy commented Nov 8, 2018

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

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

frostming added some commits Nov 8, 2018

Show resolved Hide resolved pipenv/project.py Outdated

@frostming frostming force-pushed the frostming:improve-toml-parse branch from 506f1d9 to b3aa66b Nov 8, 2018

@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

techalchemy and others added some commits Nov 8, 2018

@frostming frostming force-pushed the frostming:improve-toml-parse branch from 1b807fa to 65d7090 Nov 8, 2018

@frostming

This comment has been minimized.

Collaborator

frostming commented Nov 8, 2018

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

@techalchemy

This comment has been minimized.

Member

techalchemy commented Nov 8, 2018

Marking as WIP for now

@techalchemy

This comment has been minimized.

Member

techalchemy commented Nov 8, 2018

what is the weird handling?

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

@frostming

This comment has been minimized.

Collaborator

frostming commented Nov 9, 2018

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

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

@frostming

This comment has been minimized.

Collaborator

frostming commented Nov 9, 2018

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

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

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

This comment has been minimized.

@uranusjr

uranusjr Nov 9, 2018

Member

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:).

This comment has been minimized.

@techalchemy

techalchemy Nov 10, 2018

Member

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

Show resolved Hide resolved pipenv/project.py Outdated

frostming added some commits Nov 9, 2018

@frostming

This comment has been minimized.

Collaborator

frostming commented Nov 10, 2018

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

@frostming frostming force-pushed the frostming:improve-toml-parse branch from 88ad74c to 1f7c9ef Nov 10, 2018

@techalchemy

This comment has been minimized.

Member

techalchemy commented Nov 10, 2018

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

This comment has been minimized.

Collaborator

frostming commented Nov 10, 2018

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

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

This comment has been minimized.

@uranusjr

uranusjr Nov 10, 2018

Member

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).

This comment has been minimized.

@frostming

frostming Nov 10, 2018

Collaborator

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

This comment has been minimized.

@techalchemy

techalchemy Nov 10, 2018

Member

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

This comment has been minimized.

@techalchemy

techalchemy Nov 10, 2018

Member

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

@techalchemy

This comment has been minimized.

Member

techalchemy commented Nov 11, 2018

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

@frostming frostming removed the DO NOT MERGE label Nov 13, 2018

@techalchemy

This comment has been minimized.

Member

techalchemy commented Nov 13, 2018

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

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

Merge remote-tracking branch 'frostming/improve-toml-parse' into main…
…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>

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

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