Parsing & constructing GitHub repository URLs & specifiers

For my first not-completely-trivial Rust project, I decided to convert one of my smaller Python projects to Rust. It's a library for parsing GitHub URLs and "owner/repo" strings into objects that store the individual owner & repository name, with methods for reconstructing the various GitHub URL formats. Also included is a small command-line program for printing the owner & name of the GitHub repo for a local clone, optionally with all the various URLs as well.

The code: https://github.com/jwodder/ghrepo-rust

Aside from general comments, I'd like reviews to address the following:

  • Am I using &str vs. String (and borrowing in general) properly & idiomatically?

  • Should the tests be in lib.rs or in tests/? While I realize that, if you were to ask five different programmers about the difference between unit tests and integration tests, you'd get six different answers, the tests here really feel like unit tests to me; the only thing integration-like is that they only test the public behavior (not that there's much private behavior).

  • Is the naming of from_str_with_owner() idiomatic? The only other way I thought of to implement this function was with a dedicated DefaultOwner struct with a parse(&str) -> GHRepo method, but that seemed excessive.

  • Should LocalRepo's constructors verify that the directory exists and/or is a Git repo?

  • The API guidelines discourage the naming of getters with get_*, which initially led me to rename most of the LocalRepo methods, e.g., changing get_current_branch() to just current_branch(). However, on further reflection, these methods aren't getters, so I don't know whether it would be appropriate to give them get_* names or not.

  • Is it better to put run() and Arguments in lib.rs (as recommended by ch.12.3 of The Rust Programming Language) or in main.rs, seeing as I don't see a reason to expose them to the general public?

  • Should I keep Cargo.lock under version control or not? The Cargo FAQ says that binary crates should commit it and library crates shouldn't, but this is both a binary and a library.

  • Are there any improvements I could make to the CI setup?

Thank you in advance.

Am I using &str vs. String (and borrowing in general) properly & idiomatically?

Looks fine to me. cargo clippy doesn't complain either.

Should the tests be in lib.rs or in tests/?

It's not a big deal, do whatever works for you. Tests in lib.rs are slightly faster to run in cargo test, but moving tests out of lib.rs will make the library a tiny bit faster to compile for other users, and let you not ship test code in the crates-io crate file (where tests can't be run). For such a small library it's not a big deal either way.

from_str_with_owner

Name is ok, but I find it weird that it may silently ignore the owner part.

Should LocalRepo's constructors verify that the directory exists and/or is a Git repo?

Probably not, because you can't guarantee it will remain existing.

Is it better to put run() and Arguments in lib.rs

Don't put any APIs in lib.rs that you don't commit to supporting. It's a good idea to put them in main.rs, as it will reduce amount of code for library-only users.

Should I keep Cargo.lock

Yes. It's a long-term insurance against your dependencies breaking. For example, if any of them makes a breaking change or is yanked, but you want to bisect your history. It'd be super annoying if you needed to fix compilation on every step.

1 Like

Should LocalRepo's constructors verify that the directory exists and/or is a Git repo?

Probably not, because you can't guarantee it will remain existing.

Good point!

Should I keep Cargo.lock

Yes. It's a long-term insurance against your dependencies breaking. For example, if any of them makes a breaking change or is yanked, but you want to bisect your history. It'd be super annoying if you needed to fix compilation on every step.

Further research reveals that Cargo.lock has no effect by default when a user installs the crate, so that eliminates my only remaning concern about Cargo.lock. Thanks for the input!