Skip to content
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

WIP: Implement Add Command #223

Closed
wants to merge 16 commits into from
Closed

Conversation

0101coding
Copy link
Contributor

@0101coding 0101coding commented Sep 25, 2022

Hi @cnpryer This is another Draft PR for review. What do you think is remaining

Copy link
Owner

@cnpryer cnpryer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. A couple comments we can address. Then it's up to you how much more you want to do. I'm okay merging this incomplete if you want. I can finish the rest if it's easier.

The toml serialization should be simple. It'll change after #197 though. we also dont have --dev implemented. A test in huak::ops::add for add_project_dependency would be good too.

Lmk what you think.


unimplemented!()
let dependency = args.get_one::<String>("dependency");
add_project_dependency(dependency.unwrap().to_string(), false)?;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll checkout add_project_dependency, but can we use the same signature (or roughly) that we use for ops::remove::remove_project_dependency?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so

Suggested change
add_project_dependency(dependency.unwrap().to_string(), false)?;
if let Err(e) = ops::add_project_dependency(&project, dependency) {
return Err(CliError::new(e, ExitCode::FAILURE));
}

Comment on lines +7 to +20
pub fn add_project_dependency(package: String, _is_dev: bool) -> CliResult<()> {
let path = format!("https://pypi.org/pypi/{}/json", package);
let resp = reqwest::blocking::get(path)?;
let json: PyPi = resp.json()?;
// Get the version
let version = json.info.version;
let name = json.info.name;
let dep = PythonPackage::new(name.as_str(), None, Some(version.as_str()));
// dep.name = name;
// dep.version = version;
let venv = Venv::default();
venv.install_package(&dep)?;
Ok(())
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn add_project_dependency(package: String, _is_dev: bool) -> CliResult<()> {
let path = format!("https://pypi.org/pypi/{}/json", package);
let resp = reqwest::blocking::get(path)?;
let json: PyPi = resp.json()?;
// Get the version
let version = json.info.version;
let name = json.info.name;
let dep = PythonPackage::new(name.as_str(), None, Some(version.as_str()));
// dep.name = name;
// dep.version = version;
let venv = Venv::default();
venv.install_package(&dep)?;
Ok(())
}
pub fn add_project_dependency(project: &Project, dependency: &str, _is_dev: bool) -> Result<(), HuakError> {
let url = format!("https://pypi.org/pypi/{}/json", dependency);
let res = match reqwest::blocking::get(url) {
Ok(it) => it,
Err(e) => return Err(HuakError::AnyhowError(anyhow::format_err!(e))),
};
let json: PyPi = match res.json() {
Ok(it) => it,
Err(e) => return Err(HuakError::AnyhowError(anyhow::format_err!(e))),
};
// Get the version
let version = json.info.version;
let name = json.info.name;
let package = PythonPackage::new(name.as_str(), None, Some(version.as_str()));
match project.venv() {
Some(v) => v.install_package(package),
_ => return Err(HuakError::VenvNotFound),
}
// Remaining code to add to project's toml and serialize to pyproject.toml
// ...
Ok(())
}

Would be super cool to add HuakError::PyPIError if that makes sense.

@cnpryer
Copy link
Owner

cnpryer commented Sep 27, 2022

I'll close this out in favor of #227

@cnpryer cnpryer closed this Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants