Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
extract library #314
extract library #314
Changes from 10 commits
ae3313e
d3dc47b
edda25a
62286f2
3777e0a
039b163
1d72ef7
c1ba81a
fb3f9d2
4a495fd
2ef3486
7712fae
94bd75f
9e9d3c6
afe48e1
b4e9aa6
b27b7ab
daa8426
1190770
1324805
9d35505
c5a7ae8
b3d22d6
ad9d81c
8363bd8
368ee5b
f626d7e
19ba58f
ab2dd0a
eac165e
64509d7
9990b69
5a334ba
a496d42
bae60ec
b0f7102
371ccab
72ee617
2449acd
8485cc5
4988c0a
a6bd76b
e5da9d8
22c0081
7516ab0
09bd405
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this different than
GlobalConfig.log_level
? I'm confused by the duplication.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 decided not to expose GlobalConfig to the user. I'm only exposing the log level.
That's because if the user wants to use
cargo-semver-checks
as a library, I imaginecargo-semver-checks
won't write to stdout or stderr in the future. So it doesn't make sense to allow the user to customize stdout and stderr.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 tried to group conflicting cli arguments together in enums.
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 saw that this rustdoc also requires the Baseline rustdoc.
At the moment this is not captured by the type system.
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.
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:
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.
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.
So we want to create a RustdocSource struct that can be used both by the Baseline and Current rustdoc?
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?
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 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 theD
either.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.
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 thebaseline
's rustdoc, so now it's possible to create thecurrent
rustdoc from registry and from git revision -- yourRustdocSource
now is also fully valid for thecurrent
crates.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.
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.
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.
If you could help with the merge, that would be great @tonowak. Probably the highest impact next thing you can do right now, TBH.
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.
Ok, I'll do it in max few days.
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.
Done.
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.
Can we define explicitly what happens if
ScopeSelection::Packages
contains a package that's also inexcluded_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.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.
In 1190770 I have made it impossible for this to happen
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 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
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 not sure about this.
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.
Agreed, it doesn't seem great. Perhaps the rustdoc-source restructuring might help?
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.
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])
toCheck
, but now when I look at thelib.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.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.
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.
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.
In 7ecf587 it seems like there's still an
unknown
branch. That looks like it could work ifcurrent
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?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.
Done in 7516ab0.