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

PythonPackage parse from string++ #253

Merged
merged 8 commits into from
Sep 29, 2022
Merged

PythonPackage parse from string++ #253

merged 8 commits into from
Sep 29, 2022

Conversation

dpgraham4401
Copy link
Contributor

@dpgraham4401 dpgraham4401 commented Sep 29, 2022

howdy, (retry)

closes #199

The scope of this pull request got a little bigger than I intended it to, and the PythonPackage was pretty much completely re-implemented. While all test are passing on my end, if this causes problems or just simply isn't what you envisioned, don't feel bad about saying so.

here's what we got in this puppy...

  1. Implement PythonPackage.op as a new enum (VersionOp)
  2. New enum VersionOp
  3. PythonPackage private "string" field is replaced by all aspects of the struct field (the string field is gone)
  4. The following trait implementation
    • VersionOp
      • Default
      • std::str::FromStr
      • fmt::Display
    • PythonPackage
      • fmt::Display
      • re-implement ::from method so let foo PythonPackage::from("requests==2.28.1".to_string()); parses the python package/dependency from the string.
  5. Documentation for many pub structs and functions
  6. New test and some todo/placeholder tests

just realize I didn't re-implement PythonPackage's string function, it returns the name currently. we can fix that though.

Like i said, it's a lot so if you have questions or it's just outright in conflict with future plans, just let me know.

I would not consider myself a "rustacean" so there may be plenty of way to improve/make more idiomatic.

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.

Honestly I love it. LGTM. If you don't have any other comments I think we can merge. The only thing I'm not 100% on is the extern crate piece in lib.rs.

src/huak/ops/install.rs Show resolved Hide resolved
src/huak/lib.rs Outdated Show resolved Hide resolved
@cnpryer
Copy link
Owner

cnpryer commented Sep 29, 2022

Tag #128

@cnpryer cnpryer merged commit 9cbb0db into cnpryer:master Sep 29, 2022
Copy link
Contributor Author

@dpgraham4401 dpgraham4401 left a comment

Choose a reason for hiding this comment

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

Cool, looks more idiomatic, this is a learning experience for me.

@cnpryer
Copy link
Owner

cnpryer commented Sep 29, 2022

Cool, looks more idiomatic, this is a learning experience for me.

Same 😅. I really like it though so far.

@dpgraham4401 dpgraham4401 deleted the dep_parse branch September 29, 2022 20:39
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.

Parse name and version from dependency string
2 participants