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 17 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.
Is Rust doc comment style to include or omit the parens when naming functions? I genuinely am not sure, we should probably mirror what
std
does here.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.
By looking at this page, it seems that the std uses the () convention.
I changed this in a496d42
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.
We probably want a doc comment here as well.
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 5a334ba
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.
Maybe it would be better to call it
Registry
instead ofVersion
? It would make it more clear that it requires internet access to use it and from where this specific version is being taken from.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.
What about
RegistryVersion
? 😄The user when seeing
Registry
with a string might think aboutRegistry("crates.io")
for exampleThere 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.
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.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.
Naming is hard 😁
VersionFromRegistry
is more verbose, but more clear maybe. 🤔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 fine with either
RegistryVersion
orVersionFromRegistry
. 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.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.
What do you both think about 8485cc5?
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'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.
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.
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 😅
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.
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.
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 believe these are currently unimplemented in the CLI, so I'm fine with leaving a panic in this branch.
But let's make it
unimplemented!()
with a suitable message instead of a baretodo!()
, which makes it seem like we forgot to add it into the PR rather than making an explicit choice to not include it for now.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.
After the changes from tonowak the
todo
is not there anymore 👍