-
Notifications
You must be signed in to change notification settings - Fork 8
Native deps sync + native_package_install fixes #22
Conversation
ento
commented
Nov 16, 2016
- Adds native_deps_sync.py, counterpart of elm_deps_sync.py for elm-native-package.json
- Fixes native_package_install:
- don't install a package if the tarball is already downloaded
- don't generate trailing whitespaces when updating elm-package.json
- python 3 compatibility
…ch the nomenclature used in elm compiler
import argparse | ||
|
||
import elm_package |
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.
I was edging against this so that the file could be run by itself without needing the whole repo, but maybe it doesn't matter
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.
Yea I guessed so :p I didn't see myself downloading a single file without the option to easily upgrade: I'd do either pip or a git clone
|
||
|
||
# Change = str | ||
# sync_deps(from_deps: Dict, to_deps: Dict) -> (List[Change], Dict) |
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.
we can use actual mypy comments here like this
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.
👍
{ | ||
"elm-lang/core": "4.0.5" | ||
} | ||
''' |
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.
generally I stick to double quotes for docstrings as per PEP 257
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.
👍
(messages, new_deps) = elm_package.sync_deps(top_level, spec) | ||
spec = new_deps | ||
|
||
if len(messages) > 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.
🎨 maybe flip this -> if len(messages) == 0: print and return
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.
👍
if len(messages) > 0: | ||
print('{number} packages changed.'.format(number=len(messages))) | ||
|
||
if not dry: |
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.
maybe also flip this
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.
👍
from urllib.request import urlopen | ||
except ImportError: | ||
# Fall back to Python 2's urllib2 | ||
from urllib2 import urlopen |
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.
💯
'version': version | ||
}) | ||
|
||
return result | ||
|
||
|
||
def format_vendor_dir(base, namespace): | ||
def ensure_vendor_user_dir(base, user): |
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.
techincally it's not just user - it's also org (hence namespace)
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.
tried renaming to owner
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.
only small changes but otherwise looks good!
@eeue56 add some commits. look reasonable now? |
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.
great stuff!
thanks! |