Skip to content

Conversation

moxian
Copy link
Contributor

@moxian moxian commented Mar 31, 2025

Thanks for the cool tool!

I tried using it, and it kinda worked, but also kinda didn't. So I tweaked it a bit to make it a tiny bit better.
And I heard you liked PRs so I made one for you.

Changes neatly split into individual commits with (hopefully) descriptive messages, but I'm happy to split up the whole PR as well if that helps.

moxian added 8 commits March 30, 2025 22:25
markdown made a breaking change between 1.0.0-alpha.20 and 1.0.0-alpha.21
which made genemichaels stop building.

genemichaels never updated (as a library) to fix that,
so if we want to continue using it we have to ping markdown.

The error:

error[E0599]: no variant or associated item named `BlockQuote` found for enum `Node` in the current scope
   --> [path-to]\genemichaels-0.1.21\src\comments.rs:633:15
    |
633 |         Node::BlockQuote(x) => {
    |               ^^^^^^^^^^ variant or associated item not found in `Node`
    |
help: there is a variant with a similar name
    |
633 |         Node::Blockquote(x) => {
    |               ~~~~~~~~~~
clap formats the FromStr::Err's arising from parsing the flags
with the Display formatter. This is a very reasonable default
but it also means it loses all the context, since anyhow::Error
only prints the outermost error in Display.
So any failure parsing/creating/loading the function is displayed
to the user as simple "compiling and loading rust function", which
is impossible to debug.

Using the Debug formatting in impl FromStr for RustFunction helps
work around this.
The pass used to (?) track invalidated files itself,
but now that functionality has been moved up one level,
but also kinda not really.

So here we clarify this by:
- making reaper not care about tracking invalidated files anymore
- making processor yes care about tracking invalidated files, and
    ensuring that it does not call process_file again after gettin
    ProcessState::FileInvalidated, as it advertizes to do.
Currently all the use items have the same AstPath,
which means that privatizer tries to change all of them at once.
Which means that if *any* of them can't get privated, then
*all* of them stay unprivated, with all the consequences..
The test in the previous commit now passes
@moxian moxian force-pushed the touch branch 3 times, most recently from 942d50f to 6c67c7e Compare March 31, 2025 10:50
moxian added 7 commits March 31, 2025 04:07
Naively, if we have items (1) foo, (2) foo::bar, (3) foo::baz,
we would try to first try to remove all three of them,
and then if that fails, try to remove just (1) and (2)

..except we should already know that that would fail since it still
includes (1) which covers all three items anyway.

So, try to be smarter about this and not do that.

This is achieved by avoiding ever putting both foo and foo::whatever
in the same try-set, as that gives no information on foo::whatever
regardless of if the check passes or fails
It transforms "use std::io::{Read, Write}" into
"use std::io::Read; use std::io::Write"

Which later allows to remove just precisely the statements
we do not need, and leave rest be.

The test from the last commit does not pass, but that
is seemingly to do with the test harness setup, since it
works fine locally.
Because reaper is finnicky and does not always
work for whatever reason.

The new test from a couple commits ago now passes.
I thought it was a good idea, but now that reaper is
not completely busted, i see that this is taking a bit
longer than i'd like.. It's easier to just run reaper again
@Noratrieb
Copy link
Owner

thanks, this is awesome, both the changes and the beautiful commits :3 i'll add the pass exclusion to the docs after merging.

@Noratrieb Noratrieb merged commit 2efee49 into Noratrieb:main Apr 20, 2025
5 checks passed
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.

2 participants