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

Add flag to get current rustdoc from registry #273

Closed

Conversation

tonowak
Copy link
Collaborator

@tonowak tonowak commented Jan 8, 2023

Todo list:

  • generalize src/baseline.rs to allow generating the rustdoc with different ways for both current and baseline. Do that by renaming the file src/baseline.rs and variables in the code (the filename and variables suggest that it is a code only for the baseline, but this PR will use this code also for the current).
  • add the functionality in src/main.rs to generate current rustdoc from registry. A possible blocker is the --workspace flag.
  • add the flag (what should be its name?).

This PR is stacked on top of #270.
It allows #207 to be shorter.

@tonowak tonowak marked this pull request as draft January 8, 2023 22:57
@tonowak
Copy link
Collaborator Author

tonowak commented Jan 8, 2023

I'm not sure I like all the renames in 708c016. For example, the rustdoc_generation.rs name makes sense, but in truth, the rustdoc is generated in dump.rs (which, by the way, isn't a descriptive name). Nevertheless, I think the new names make more sense than previously and they help in understanding what exactly the code does.

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

I feel like this is trying to force the wrong abstraction through. This new API feels overly complex and not particularly elegant. I suggested an alternative approach in one of the comments.

Comment on lines +91 to +93
let metadata = args.manifest.metadata().no_deps().exec()?;
let target_path = metadata.target_directory.as_std_path().join(util::SCOPE);
let source_path = metadata.workspace_root.as_std_path();
Copy link
Owner

Choose a reason for hiding this comment

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

Any particular reason to move these out of their original block and make them always get executed? I'm confused.

@@ -123,8 +118,17 @@ fn main() -> anyhow::Result<()> {
.silence(!config.is_verbose());

let mut success = true;
let mut run_check_release = |config: &mut GlobalConfig, crate_name: &str, current_crate: VersionedCrate, baseline_crate: VersionedCrate| -> anyhow::Result<()> {
if !check_release::run_check_release(config, crate_name, current_crate, baseline_crate)? {
let mut run_check_release = |config: &mut GlobalConfig,
Copy link
Owner

Choose a reason for hiding this comment

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

Very much not a fan of complex inline function definitions like this. It is unidiomatic and makes the code difficult to read. The reader has to jump back and forth between the uses and the definition of the closure, which also has complex arguments and touches external state, while not even including a doc comment.

Please try to find another way to structure the code with readability and ease of understanding in mind.

Comment on lines +11 to +17
fn generate_rustdoc(
&self,
config: &mut GlobalConfig,
rustdoc: &RustDocCommand,
rustdoc_command: &RustDocCommand,
name: &str,
version_current: Option<&semver::Version>,
highest_considered_registry_version: Option<&semver::Version>,
crate_type: &str,
Copy link
Owner

Choose a reason for hiding this comment

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

At this point, I feel like this trait has gotten complex enough that it needs documentation on what all its arguments mean. It's not obvious to me what crate_type is supposed to be, and highest_considered_registry_version has a specificity to it that makes me afraid that it might be more complex than it might seem at first glance.

) -> anyhow::Result<std::path::PathBuf>;
}

pub(crate) struct RustdocBaseline {
pub(crate) struct FromRustdoc {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm overall not a fan of the From prefixes in all these names. While rustdoc_generation::FromRustdoc etc. seems pretty obvious, there's zero guarantee that it will always be used like that and it seems brittle from an understandability perspective to assume it. For example, Debug would only print the name of the type itself.

Consider the RustdocFrom* prefix? Then this struct could be RustdocFromFile etc.

On a separate note, I'm astonished to see that these structs aren't #[derive(Debug)]. Please add that throughout?

Comment on lines -274 to +289
fn choose_baseline_version(
fn choose_version_from_registry(
crate_: &Crate,
version_current: Option<&semver::Version>,
highest_considered_registry_version: Option<&semver::Version>,
Copy link
Owner

Choose a reason for hiding this comment

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

This strongly makes me feel like we're trying to force the wrong abstraction here. highest_considered_registry_version is only relevant for baselines, and specifically for registry-based baselines, and yet we're carrying it throughout the entire API including in the trait etc.

Perhaps take a step back from this and consider another approach? Maybe one where the baseline generation portion is aware it's generating a baseline (and can show appropriate status and error messages for it!), and a lower-level abstraction that just generates a rustdoc JSON file based on the information it's given?

@tonowak
Copy link
Collaborator Author

tonowak commented Jan 9, 2023

I'm thinking about closing this PR and adding the functionality I had in mind through this PR to the #207. That'll create a clear line between cargo-semver-checks --a tool to analyze own's code, and #207 -- a tool to analyze results for chosen crates and versions (which will be used e.g. to test for false-positives of cargo-semver-checks in the CI).

What do you think about it?

Assuming you agree with closing this PR, should I nevertheless refactor src/baseline.rs in any way, or should I disregard all changes?

@obi1kenobi
Copy link
Owner

Assuming @epage is okay with my suggestion in #86 (comment), I think we'll still need to extract the code for generating rustdoc based on a registry crate's name and version into something the new tool can also call.

It's likely that those changes would look a bit different than the proposed refactor in this PR.

I'd recommend closing this draft PR, opening a new PR for those changes by themselves, and keeping them separate from #207 just in the interest of keeping the components small and easy to review. Ideally, #207 shouldn't have any effect on the semver-checks tool's portion of the code — that would make it very easy to read and merge.

@tonowak tonowak closed this Jan 9, 2023
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.

3 participants