-
Notifications
You must be signed in to change notification settings - Fork 7
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
Update remote of local only #22
base: master
Are you sure you want to change the base?
Conversation
@jayvdb I have updated the project master to include our standard set of linters and formatters. Could you rebase this PR on top of the new master? |
2af6508
to
da24870
Compare
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.
One small change requested.
|
||
def remote() -> Dict[str, List]: | ||
|
||
def remote(packages: List[PackageVersion] = []) -> Dict[str, List]: |
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.
Update to remove mutable default args (see https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments):
def remote(packages: List[PackageVersion] = []) -> Dict[str, List]: | |
def remote(packages: List[PackageVersion] = None) -> Dict[str, List]: |
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 had that originally; it causes lots of mypy problems, which requires adding noqa
or worse voodoo to suppress. Happy to do it if you still want it..
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 you could, yes. In situations like this I always defer to "what would I think if this code was projected in size 128px font a conference" - and in this case I'd be more embarrassed about the mutable args than I would about # type: ignore
.
Revisiting this after many years (we don't use it any more), this whole module needs a rethink - remote
should just take a list of PackageVersion
s, it shouldn't be optional at all. If you could fix the args issue I'll merge this and release a version. I can rework the implementation at some point in the future.
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.
fwiw, no need to rush out a new version. I have other pending PRs to create, and they should be fine to squeeze into the current release.
da24870
to
d22e659
Compare
If --local and --remote used, do not refresh PyPI remote data for any stale records which were not found in local data.
d22e659
to
d51380b
Compare
In effect, this allows enabling local and remote and not clearing, and the update wont try remote sync all of the packages which are stale (not in local)