extract library #314

MarcoIeni · 2023-01-21T22:43:46Z

A step towards #67
To keep things easier I just created a new lib.rs
In the future, we could extract this lib.rs file into a core crate, that doesn't depend on clap.

MarcoIeni · 2023-01-21T22:45:14Z

src/lib.rs

+    /// Latest version published to the cargo registry.
+    #[default]
+    LatestVersion,
+}


I tried to group conflicting cli arguments together in enums.

MarcoIeni · 2023-01-21T22:46:30Z

src/lib.rs

+    /// It can be a workspace or a single package.
+    Manifest(PathBuf),
+    /// The rustdoc json of the current version of the project.
+    RustDoc(PathBuf),


I saw that this rustdoc also requires the Baseline rustdoc.
At the moment this is not captured by the type system.

How would you deal with this?

Should we move it outside of this enum?

All of these allow the baseline rustdoc to be specified explicitly, while only one option requires it. This makes the decision tricky. It feels like this enum isn't at the right level of abstraction.

The baseline and the current rustdoc can be one of three things:

already generated, just load it from an existing file

generate it based on code at some path, optionally first checking out a gitrev (maybe better split into two cases? what do you think?)

generate it based on a registry version (in the current CLI, only the baseline can do this, but @tonowak has a lib use case where it would be useful for the current rustdoc as well)

Perhaps a pub enum RustdocSource like that might be better? Then the CLI can have its own structs and behaviors around "current directory" etc. For testability reasons, it might be best if the library is ignorant of the current directory and always expects explicit paths to be fed to it.

Not set on it, just wanted to suggest an idea to look at.

So we want to create a RustdocSource struct that can be used both by the Baseline and Current rustdoc?

already generated, just load it from an existing file

enum RustdocSource { RustDoc(PathBuf) }

generate it based on code at some path, optionally first checking out a gitrev (maybe better split into two cases? what do you think?)

enum RustdocSource { RustDoc(PathBuf), Root(PathBuf), // (path to root, rev) Revision(PathBuf, String) }

generate it based on a registry version

enum RustdocSource { RustDoc(PathBuf), Root(PathBuf), // (path to root, rev) Revision(PathBuf, String), Version(Option<String>), }

This looks similar to the current Baseline enum. Is it valid for Current, too?
If I didn't get it, can you give me the structure of the enum you were thinking about?

I think it looks reasonable. It currently isn't valid for Current in the CLI, but I think it should be in the lib. Not much reason not to: that would make the types more complex while providing less functionality.

Small nit: let's call the variant Rustdoc instead, since the underlying tool doesn't capitalize the D either.

It currently isn't valid for Current in the CLI, but I think it should be in the lib.

FYI @MarcoIeni, the recent merged chain of PRs (which ended with #351) made it so that the current crate's rustdoc is generated in the same way as the baseline's rustdoc, so now it's possible to create the current rustdoc from registry and from git revision -- your RustdocSource now is also fully valid for the current crates.

And also, this chain of PRs renamed quite a lot of things in the source code, so there might be a big merge conflict, sorry :( I can try to merge main into this branch when I'll have some free time.

If you could help with the merge, that would be great @tonowak. Probably the highest impact next thing you can do right now, TBH.

Ok, I'll do it in max few days.

src/lib.rs

MarcoIeni · 2023-01-21T22:50:42Z

src/lib.rs

+    pub fn with_manifest(&mut self, path: PathBuf) -> &mut Self {
+        self.current = Current::Manifest(path);
+        self
+    }


I used a builder pattern that matches the cli args.
style of builder pattern: https://rust-lang.github.io/api-guidelines/type-safety.html#non-consuming-builders-preferred

MarcoIeni · 2023-01-21T22:52:11Z

src/lib.rs

+            Current::Manifest(path) => path.clone(),
+            Current::RustDoc(_) => {
+                anyhow::bail!("error: RustDoc is not supported with these arguments.")
+            }


I'm not sure about this.

Agreed, it doesn't seem great. Perhaps the rustdoc-source restructuring might help?

src/lib.rs

src/config.rs

obi1kenobi

Very excited about this, thanks for putting it together!

obi1kenobi · 2023-01-22T04:42:31Z

src/lib.rs

+    /// It can be a workspace or a single package.
+    Manifest(PathBuf),
+    /// The rustdoc json of the current version of the project.
+    RustDoc(PathBuf),


All of these allow the baseline rustdoc to be specified explicitly, while only one option requires it. This makes the decision tricky. It feels like this enum isn't at the right level of abstraction.

The baseline and the current rustdoc can be one of three things:

already generated, just load it from an existing file

generate it based on code at some path, optionally first checking out a gitrev (maybe better split into two cases? what do you think?)

generate it based on a registry version (in the current CLI, only the baseline can do this, but @tonowak has a lib use case where it would be useful for the current rustdoc as well)

Perhaps a pub enum RustdocSource like that might be better? Then the CLI can have its own structs and behaviors around "current directory" etc. For testability reasons, it might be best if the library is ignorant of the current directory and always expects explicit paths to be fed to it.

Not set on it, just wanted to suggest an idea to look at.

src/lib.rs

obi1kenobi · 2023-01-22T04:48:03Z

src/lib.rs

+            Current::Manifest(path) => path.clone(),
+            Current::RustDoc(_) => {
+                anyhow::bail!("error: RustDoc is not supported with these arguments.")
+            }


Agreed, it doesn't seem great. Perhaps the rustdoc-source restructuring might help?

src/lib.rs

obi1kenobi · 2023-01-22T04:51:12Z

src/lib.rs

+#[derive(Default)]
+struct Scope {
+    selection: ScopeSelection,
+    excluded_packages: Vec<String>,
+}
+
+/// Which packages to analyze.
+#[derive(Default, PartialEq, Eq)]
+enum ScopeSelection {
+    /// Package to process (see `cargo help pkgid`)
+    Packages(Vec<String>),


Can we define explicitly what happens if ScopeSelection::Packages contains a package that's also in excluded_packages? I'm not sure what should happen, or what cargo does assuming the same thing can happen in cargo. But our lib docs should state the expected behavior in any case.

In 1190770 I have made it impossible for this to happen

src/lib.rs

Co-authored-by: Predrag Gruevski <[email protected]>

tonowak · 2023-01-25T15:40:59Z

Hi @MarcoIeni! It turns out that to complete #207 I need exactly what you're doing in this PR -- allowing the tool to be usable as a library. Thus, if you think it would be helpful, I would be happy to help you with this PR. What do you think? Is there anything in particular I could help you with, or are you progressing nicely and don't need any help?

MarcoIeni · 2023-01-25T21:06:53Z

Hey @tonowak, if I'm blocking you, feel free to contribute to this pr, yes 👍
For example, today I won't have time to look into this.
Feel free to commit directly into the branch, or to raise prs against it. 👍

I was thinking that with the type system, we could avoid the possibility of specifying packages together with the other options.
However, excluded_packages should be compatible with "workspace" or "default members" options.

src/lib.rs

MarcoIeni · 2023-02-08T17:13:32Z

Thanks for helping Tomasz 😄
I think I have addressed all the PR comments. I won't edit this PR anymore unless you still need something else from my side 👍

tonowak · 2023-02-08T17:54:54Z

src/lib.rs

+    /// If `None`, uses the largest-numbered non-yanked non-prerelease version
+    /// published to the cargo registry. If no such version, uses
+    /// the largest-numbered version including yanked and prerelease versions.
+    Version(Option<String>),


Maybe it would be better to call it Registry instead of Version? It would make it more clear that it requires internet access to use it and from where this specific version is being taken from.

What about RegistryVersion? 😄
The user when seeing Registry with a string might think about Registry("crates.io") for example

Yeah, that might be better 👍 .

... but going with this logic, the user might think that in RegistryVersion one has to pass the version of the registry to use, not the version of the crate to compare with.

Naming is hard 😁
VersionFromRegistry is more verbose, but more clear maybe. 🤔

I'm fine with either RegistryVersion or VersionFromRegistry. I'm not that worried about people thinking that they need to pass a version of a registry, but I'm willing to accept I may be overly optimistic if you think the other option is better.

What do you both think about 8485cc5?

I think it's fine :)
I would not try to make the library perfect in this PR. Otherwise we will have to deal with too many merge conflicts.

I guess (and hope) that the library will be extracted in a separate crate, so we can still change the library API in future PRs.

This PR is on my to-do list to review today. Sorry for the delay! Trustfall ended up in the spotlight (it was GitHub's #1 most-starred project a couple of days ago) and I've been super busy over there for the last couple of days.

I know I probably just merged another PR that causes another merge conflict, and I'm happy to fix the mess of my own making 😅

tonowak · 2023-02-09T10:08:25Z

src/lib.rs

+            RustdocSource::Rustdoc(_)
+            | RustdocSource::Revision(_, _)
+            | RustdocSource::VersionFromRegistry(_) => {
+                let name = "<unknown>";


While developing #207 I've found a bug: when both crates are generated from registry, this code tries to find a package named <unknown> in registry. From how the interface looked, I was convinced that it is enough to pass .with_packages(vec![name]) to Check, but now when I look at the lib.rs, I don't think it's what it's supposed to accept. So, in the scenario "from registry & from registry", there is no way to pass the name of the crate to check.

In #207 I've made a temporary fix 7ecf587 so that I can progress further. In the case where this fix is good enough, I can move it here, otherwise we should discuss better alternatives.

In 7ecf587 it seems like there's still an unknown branch. That looks like it could work if current is given by gitrev, but won't work with a registry version nor with a premade rustdoc file.

Can we add an explicit panic!() in the cases we know for sure won't work, instead of setting a bogus value and then breaking in a confusing way?

Done in 7516ab0.

obi1kenobi

This looks in pretty good shape. Let's target merging tomorrow or Monday morning if possible, so I can include this in the next release (also planned for Monday at the moment).

obi1kenobi · 2023-02-12T03:48:15Z

src/lib.rs

+            RustdocSource::Rustdoc(_)
+            | RustdocSource::Revision(_, _)
+            | RustdocSource::VersionFromRegistry(_) => {
+                let name = "<unknown>";


In 7ecf587 it seems like there's still an unknown branch. That looks like it could work if current is given by gitrev, but won't work with a registry version nor with a premade rustdoc file.

Can we add an explicit panic!() in the cases we know for sure won't work, instead of setting a bogus value and then breaking in a confusing way?

obi1kenobi · 2023-02-12T03:50:32Z

src/lib.rs

+            } else if let Some(path) = get_target_dir_from_project_root(&self.baseline.source)? {
+                path
+            } else {
+                get_xdg_cache_dir()


I suspect this won't work on Windows, unfortunately. Let's instead use the directories crate which is the known-working cross-platform way to do this:

if let Some(proj_dirs) = ProjectDirs::from("cargo-semver-checks") { proj_dirs.config_dir(); // probable dirs (still need to be created! the library won't make them for us) // Lin: /home/alice/.config/cargo-semver-checks // Win: C:\Users\Alice\AppData\Roaming\cargo-semver-checks\config // Mac: /Users/Alice/Library/Application Support/cargo-semver-checks }

obi1kenobi · 2023-02-12T04:10:14Z

I took care of the merge, it wasn't that bad because I reviewed the code that caused a conflict. Lmk what you think about the panic!() branch and using the cross-platform-compatible app dir crate!

Really looking forward to merging this soon! 🚀🎉

MarcoIeni · 2023-02-17T23:09:26Z

Lmk what you think about the panic!() branch and using the cross-platform-compatible app dir crate!

Is this question for me? If yes, what's the panic!() branch?

obi1kenobi · 2023-02-17T23:58:16Z

The two remaining items needed to merge this are:

This <unknown> branch that seems likely to cause an obscure error if someone hits it, and I'm recommending an explicit panic!() instead. It's based on changes @tonowak made.
Using the cross-platform directories crate instead of xdg so that our non-Unix users aren't left behind.

I unfortunately have had to shift my primary focus to Trustfall for a bit, given the recent massive uptick in interest in the project. I think the remaining items aren't much work and I trust both of you to do a good job on them.

MarcoIeni · 2023-02-18T10:25:00Z

I replaced xdg with directories. I leave the other fix to tonowak :)

tonowak · 2023-02-18T19:38:19Z

Thanks :) I'll probably do the other fix in the next 48h.

obi1kenobi

This looks polished enough to release, let's not block @MarcoIeni's release-plz work any longer. We will certainly keep improving the library APIs over time, we don't have to get to "perfect" on day 1.

Thanks, everyone!

MarcoIeni · 2023-02-21T15:33:26Z

Thanks!
This week I'll work on extracting the library into a separate crate.
In this way, the users of the library don't need to compile clap and other specific libraries used only in the cli.

Some naming ideas:

semver-checks
cargo-semver-checks-core

Please, reserve in crates.io the one you prefer :)

obi1kenobi · 2023-02-21T16:09:29Z

I grabbed both, just to be extra safe. semver-checks seems good to me, but I don't have a strong preference.

My 2 cents: splitting the library into a separate crate would definitely be nice, but it doesn't seem that critical to me. The community impact of having semver-checking made automatic via release-plz seems bigger, and we can always split the library into a separate crate and cut down compile times later.

Of course, release-plz is your crate, this is your work, and it's your call to make :)

tonowak · 2023-02-21T16:19:52Z

Maybe it would be reasonable to have the clippy and CLI under a default flag?

MarcoIeni · 2023-02-23T21:21:24Z

CLI under a default flag

This would prevent us to ship breaking changes independently for the cli and for the library.

For example, if we change a command line argument, should it be a breaking change?

Having two separate crates allows us to ship breaking changes for cli and the lib independenlty

MarcoIeni added 3 commits January 21, 2023 22:33

wip

ae3313e

wip

d3dc47b

wip

edda25a

MarcoIeni commented Jan 21, 2023

View reviewed changes

src/lib.rs Show resolved Hide resolved

MarcoIeni commented Jan 21, 2023

View reviewed changes

expand match

62286f2

MarcoIeni commented Jan 21, 2023

View reviewed changes

src/lib.rs Show resolved Hide resolved

MarcoIeni added 3 commits January 21, 2023 23:56

clippy

3777e0a

Merge branch 'main' into lib

039b163

clippy

1d72ef7

MarcoIeni commented Jan 21, 2023

View reviewed changes

src/config.rs Show resolved Hide resolved

MarcoIeni added 3 commits January 22, 2023 00:07

move match

c1ba81a

move rev

fb3f9d2

easier

4a495fd

MarcoIeni marked this pull request as ready for review January 21, 2023 23:15

obi1kenobi reviewed Jan 22, 2023

View reviewed changes

MarcoIeni and others added 2 commits January 22, 2023 09:36

Update src/lib.rs

2ef3486

Co-authored-by: Predrag Gruevski <[email protected]>

Merge remote-tracking branch 'origin' into lib

7712fae

MarcoIeni added 3 commits January 25, 2023 18:25

introduce Rustdoc struct

94bd75f

adapt code to rustdoc refactor

9e9d3c6

Merge branch 'main' into lib

afe48e1

MarcoIeni added 2 commits January 25, 2023 22:07

clippy

b4e9aa6

fix

b27b7ab

MarcoIeni commented Jan 25, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

inline function

daa8426

remove useless check

371ccab

tonowak added 2 commits February 8, 2023 18:24

Fix baseline_root when no baseline-path given and baseline-rev passed

72ee617

Fallback to XDG cache dir

2449acd

tonowak reviewed Feb 8, 2023

View reviewed changes

tonowak requested a review from obi1kenobi February 8, 2023 17:56

Renamed registry/version items

8485cc5

tonowak mentioned this pull request Feb 9, 2023

Script that runs the lints on most popular crates #207

Draft

tonowak reviewed Feb 9, 2023

View reviewed changes

obi1kenobi reviewed Feb 12, 2023

View reviewed changes

Merge branch 'main' into lib

4988c0a

Merge branch 'main' into lib

a6bd76b

MarcoIeni added 2 commits February 18, 2023 11:04

fmt

e5da9d8

use directories crate

22c0081

Added panic in 'unknown' branch

7516ab0

tonowak requested a review from obi1kenobi February 21, 2023 08:44

Merge branch 'main' into lib

09bd405

obi1kenobi approved these changes Feb 21, 2023

View reviewed changes

obi1kenobi enabled auto-merge (squash) February 21, 2023 14:17

obi1kenobi merged commit 51803af into obi1kenobi:main Feb 21, 2023

obi1kenobi mentioned this pull request Feb 24, 2023

Integrate with cargo-semver-checks release-plz/release-plz#373

Closed

extract library #314

extract library #314

Conversation

MarcoIeni commented Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

MarcoIeni Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoIeni Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonowak commented Jan 25, 2023

MarcoIeni commented Jan 25, 2023 • edited Loading

MarcoIeni commented Feb 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi commented Feb 12, 2023

MarcoIeni commented Feb 17, 2023

obi1kenobi commented Feb 17, 2023

MarcoIeni commented Feb 18, 2023

tonowak commented Feb 18, 2023

obi1kenobi left a comment

Choose a reason for hiding this comment

MarcoIeni commented Feb 21, 2023

obi1kenobi commented Feb 21, 2023

tonowak commented Feb 21, 2023

MarcoIeni commented Feb 23, 2023

MarcoIeni commented Jan 21, 2023 •

edited

Loading

MarcoIeni Jan 21, 2023 •

edited

Loading

MarcoIeni Jan 21, 2023 •

edited

Loading

MarcoIeni commented Jan 25, 2023 •

edited

Loading