-
Notifications
You must be signed in to change notification settings - Fork 6
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
Test lower bounds specified in main dependencies #70
Conversation
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.
@jdawang This is great! Thanks for the effort. I have one question, but I think your docs actually answer my question. Great experimental feature!
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 work! A couple optional fixes but I appreciate the extensive testing and documentation you put into this.
edgetest/core.py
Outdated
lower : list | ||
The list of packages to install lower bounds. |
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.
Documentation note: reading the utilities, this is a list of the packages to be lowered alongside the lower bound, correct? I.e.
[
"pandas==1.5.1",
]
instead of
["pandas",]
Should we add that to the docs where lower
is an input? It's different than the format of upgrade
.
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.
Yeah that's a good catch. In the config, it's just like upgrade
, but in TestPackage
, the config is already parsed to provide the package to be lowered alongside the lower bound.
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.
Yeah not a huge deal since it's not user facing. Might save us some confusion down the road though.
edgetest/utils.py
Outdated
output["envs"][-1]["lower"] = "\n".join( | ||
f"{pkg_name}=={lower_bound}" | ||
for pkg_name, lower_bound in get_lower( | ||
config.get("options", "install_requires") | ||
).items() | ||
if _isin_case_dashhyphen_ins( | ||
pkg_name, output["envs"][-1]["lower"].split("\n") | ||
) | ||
and lower_bound is not None | ||
# TODO: Emit warning if lower_bound is None but package is in lower | ||
# TODO: Parse through extra requirements as well to get lower bounds | ||
) |
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'm finding this loop a bit difficult to read. What if we split it out a bit? Something along the lines of
output["envs"][-1]["lower"] = "\n".join( | |
f"{pkg_name}=={lower_bound}" | |
for pkg_name, lower_bound in get_lower( | |
config.get("options", "install_requires") | |
).items() | |
if _isin_case_dashhyphen_ins( | |
pkg_name, output["envs"][-1]["lower"].split("\n") | |
) | |
and lower_bound is not None | |
# TODO: Emit warning if lower_bound is None but package is in lower | |
# TODO: Parse through extra requirements as well to get lower bounds | |
) | |
output["envs"][-1]["lower"] = "" | |
for pkg_name, lower_bound in get_lower(config.get("options", "install_requires").items(): | |
if ( | |
_isin_case_dashhyphen_ins(pkg_name, output["envs"][-1]["lower"].split("\n")) and lower_bound is not None | |
): | |
output["envs"][-1]["lower"] += f"{pkg_name}=={lower_bound}\n" | |
# TODO: Emit warning if lower_bound is None but package is in lower | |
# TODO: Parse through extra requirements as well to get lower bounds |
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.
LGTM! Great work!
Description
This is a feature that allows users to specify packages that they want to test lower bounds of. If specified in an edgetest environment, edgetest will:
>=
specifier. This is the initial implementation and we can handle extras later.Currently, there is no:
>=
, as this would require a search.lower
or vice versa. Might want to add this in right now, but can also leave for later and document.So, no testing of lower bounds will be done unless explicitly asked for by the user.
There are also some changes that I made to get it to work:
lower
that is mutually exclusive with `upgrade.Closes #69
Type of change
Checklist: