Conversation
Check command fails if the current lock would be affected by `prune`. The `check` command takes no output options since it will not write any changes, but just check that prunning results in the same *hits* (FlakeNodeVisits) as those computed for the current lock file. After computing hits for the _current_ and _pruned_ lock files, the `check` command panics if they are different. panic! was used as it would terminate and write to stderr. (elog! eprintln! macros dont write to stderr) Closes spikespaz#4
spikespaz
left a comment
There was a problem hiding this comment.
Left a few requests, and one more: could you please remove the period from the commit summary, since it's not (supposed to be) a proper sentence?
| let pruned_hits = FlakeNodeVisits::count_from_index(&prune_lock, prune_lock.root_index()); | ||
|
|
||
| if pruned_hits.inner != current_hits.inner { | ||
| panic!("Check failed: Lock file needs prunning. Run `allfollow prune` to fix it.") |
There was a problem hiding this comment.
Typo: prunning => pruning
Nitpicks -- I'm the type to agonize over wording.
I think it's good that you chose "lock file" over "flake.lock", considering the name of the input file may not be known.
In MLA style, capitalize after a colon only if:
- it introduces two or more complete sentences, or
- the following word is a proper noun.
In APA style, capitalize after a colon if:
- the following is a complete sentence, or
- the following word is a proper noun.
In this case, "lock file needs pruning" is a noun phrase, not a complete clause, and contains no proper nouns. Thus, both APA and MLA would recommend lower-cased "lock" here.
Here's a version that satisfies APA, MLA, and the rustc style guide:
Check failed: lock file needs pruning; run `allfollow prune` to fix it.
Alternatively, if you would prefer to use complete sentences, this is also valid (though it deviates from rustc conventions):
Check failed: The lock file needs pruning. Run `allfollow prune` to fix it.
I personally prefer the first version because it aligns well with the compiler's style guide, avoiding capitalization while using a semicolon for structural clarity and rhythm.
"The text should be matter of fact and avoid capitalization and periods, unless multiple sentences are needed."
| prune_orphan_nodes(&mut prune_lock); | ||
| let pruned_hits = FlakeNodeVisits::count_from_index(&prune_lock, prune_lock.root_index()); | ||
|
|
||
| if pruned_hits.inner != current_hits.inner { |
There was a problem hiding this comment.
Just a note: inner here is private, if FlakeNodeVisits moves out to library code (it may for the sake of moving tests), this field won't be accessible to the binary code. You could use */deref or into_inner.
| output_opts.overwrite = true; | ||
| } | ||
| } | ||
| _ => () |
There was a problem hiding this comment.
There is an #[allow(clippy::single_match)] on this expression, it should have been expect, but could you please remove it?
| let current_hits = FlakeNodeVisits::count_from_index(¤t_lock, current_lock.root_index()); | ||
|
|
||
| let mut prune_lock = current_lock.clone(); | ||
| substitute_flake_inputs_with_follows(&prune_lock, no_follows); |
There was a problem hiding this comment.
Realistically, this is the only thing that needs to happen for the check command to know if the flake lock would change. The reason for this is that prune_orphan_nodes will only change the graph if substitute_flake_inputs_with_follows has, which means you should be able to just compare:
let original_lock = read_flake_lock(lock_file);
let modified_lock = original_lock.clone();
substitute_flake_inputs_with_follows(&modified_lock, no_follows);
if modified_lock != original_lock {
panic!("Check failed: Lock file needs pruning. Run `allfollow prune` to fix it.");
}Any other indirection, including constructing FlakeNodeVisits, would be redundant, since you really only need to answer whether any transitive inputs can be redirected.
Since you're the first person needing the check command, and there are no automated tests (yet), please try to invalidate these claims if you can.
An added bonus: since the logging is not yet abstracted through another crate, it's nice to use as few "binary crate" functions as possible to reduce the console spam.
|
Either way, I'm not worried about the mechanism of the exit code, I plan on ripping out all the panics and using proper errors. |
|
Hi, it's been a week. I was wondering if you'd like me to complete this change, or do you want to see it through? |
|
Hey, yes please. If you do have time, please make changes as you see appropriate. I just have had not have the time to work on it. (I'm on due date to finish a white paper about my github.com/vic/fx-rs effects system, so I won't probably have much time next week either). |
Now that allfollow is [deterministic](spikespaz/allfollow#1) we no longer have to use jq to sort its output. We now use `delta` instead of `difftasitc` to fail if flake.lock was not already pruned. I'm not sure now that we need an `allfollow check` command, since it is better for users to see a nice diff as error message. Related: spikespaz/allfollow#7
Now that allfollow is [deterministic](spikespaz/allfollow#1) we no longer have to use jq to sort its output. We now use `delta` instead of `difftasitc` to fail if flake.lock was not already pruned. I'm not sure now that we need an `allfollow check` command, since it is better for users to see a nice diff as error message. Related: spikespaz/allfollow#7
Check command fails if the current lock would be affected by
prune.The
checkcommand takes no output options since it will not write any changes, but just check that prunning results in the same hits (FlakeNodeVisits) as those computed for the current lock file.After computing hits for the current and pruned lock files, the
checkcommand panics if they are different.panic! was used as it would terminate and write to stderr. (elog! eprintln! macros dont write to stderr)
Closes #4