Stateful Formatters #420
Stateful Formatters #420
Conversation
|
Note I moved the estimator to it's own module. It had to be public for this to work, and I'm not sure whether you guys would want to make the state module public, and it didn't really seem to fit anywhere else. |
|
The more I think about this, the more I wonder what other use cases somebody might have for needing this level of access to the API, and I might be over-engineering it as a result. This might be better done with allowing the user to specify whether estimations are done per tick, as the default estimator does, or over n seconds, milliseconds, etc. (milliseconds hypothetically in a scientific use case? I personally don't have a use case for sub-second sampling) I think my example merely demonstrates a case where the default estimator simply doesn't work at all (basically the numbers are completely meaningless to the user) but at the same time they'd still like and benefit from having an estimation provided. I think the efficiency improvements I've made in my alternative estimator might even make it more efficient than the existing one, as it does hardly anything at all until it's time to produce a new sample, which itself isn't all that costly over that interval, but at the same time it probably shouldn't outright replace the existing one as I could see many people preferring that per-tick estimation that is currently used, and per-tick should probably remain the default as it is much more precise. Let me know what your thoughts are, or you can outright tell me that I'm totally bonkers and I'll accept that too :) I'll hold off on any further work on the estimator itself until I hear from one of the maintainers, though I have a few other formatting improvements in mind that I might (or might not) push to this same repo because it's working on solving a very similar if not the same problem, but I'm still trying to figure out how they'll work. |
|
The last push I did gets everything working the way I need it to. Please see the rate_smoothing example for the overall effect of what it is I'm trying to do, with example real world use cases mentioned in the comments. I'm also tempted to implement a lock-free incrementing function, but I need to evaluate whether that will offer much benefit in what I'm working on. |
|
That's odd, I compiled against rustc 1.53 and there were no lints, and all tests passed. I even added some very aggressive clippy lints and they were all clear. I also ran the cargo format check exactly as those checks do. All of that was done before being pushed. I'll look at it again to see if something slipped by somehow. |
|
It's very possible that clippy has gotten more strict since 1.53, especially if you enable all those extra strict stuff. 1.53 is pretty old at this point, maybe upgrade to current stable? |
|
Oh I thought those tests were specifically against 1.53? Sure, I'll use the latest stable, I used rustup to override to 1.53 so that I could guarantee compatibility with 6 months prior as you had asked in my other PR. |
|
We run the tests against 1.53 to ensure the library works well, but all the linking happens on stable. |
|
For this to be mergable, I'd like to:
As I've tried to explain a few times, this should be a minimal change. Also, it would make my life easier if discussion of the relevant changes happened in this PR rather than in the issue. |
|
Not sure what you mean move it 'back'? The format_map is still in progressstate. That particular map I made into a dashmap had to be that way because it originally did live in progressstate, because it provides internal mutability (it was Arc, hence why those variables were named 'adm'.) It doesn't need to be dashmap if it's in progressstate though. While I can put some sort of default in there, I'm not sure where you are wanting it to be called from. The only other thing I can do is require that statefulformatter is immutable with internal mutability, which complicates things a lot from the user side as it will probably need atomics even if they're not really appropriate for the use case, or it forces them into using locks. I also don't really know how to meet my own use case while falling within those constraints. I'm looking it over to see what can be done, but right now I don't know. |
Sorry, I meant we should not add a map to
From the
Why? |
Right but in order to live inside of progressstyle it has to be Clone. I don't know how to do that while also making it Send + Sync by adding Arc, which removes mutability. Unless progressstyle isn't meant to be cloned across threads? |
|
You can remove the derived Clone impl in favor of manual Clone impl, that gives you the flexibility to do something different for the format_map values. |
|
Ah I see. I'll chew on this for a bit. |
|
I'm still not really understanding the use case for cloning ProgressStyle. When ProgressStyle is cloned, how do we resume the formatter state from the existing instance? How do we keep the new one syncronized? Unless calling default is really supposed to be just another way of calling clone? But then I don't understand how the clone stays syncronized. Or is it that when people do clone it, they already intend on resetting something? |
|
Well whatever the use case is, it doesn't seem to impact mine at all. Still a draft, there are a few things I need to clean up, but overall let me know if this works. |
|
I think I'm mostly satisfied with what I have there. I'd really like the ability to remove or at the very least suppress the built-in estimator as it will be running (I think? unless the compiler sees it as dead code somehow and removes it anyways) even though my use case doesn't need it at all. Also I still don't understand the need for cloning ProgressStyle, but it doesn't interfere with anything I'm doing in any way so no problem there. If you don't count my example code, or the comments, or the extra lines mandated by cargo fmt, this pull request is somewhere under 80 lines. For quick reference, here's the trait signature: pub trait StatefulFormatter: Display + Send {
fn default(&self) -> Box<dyn StatefulFormatter>;
fn tick(&mut self, state: &mut ProgressState, now: Instant);
fn reset(&mut self, state: &mut ProgressState, now: Instant);
}The reason I added progressstate to reset is so that if there is only a partial reset as opposed to a full reset, we can figure that out and adjust accordingly. |
examples/rate_smoothing.rs
Outdated
| @@ -0,0 +1,218 @@ | |||
| #![warn(clippy::pedantic)] | |||
There was a problem hiding this comment.
I'd prefer not to add an example for this right now.
There was a problem hiding this comment.
this will be removed last, let's just make sure everything else is good first
There was a problem hiding this comment.
Let's remove this, you can just keep it in your local working dir for testing.
There was a problem hiding this comment.
I'm going to keep this until everything else looks good. The reason why is because I want to continually evaluate whether the syntax I'm able to use looks correct and idiomatic. It acts as a sanity check, and when I refactor anything crate side, it auto-refactors on my side as well
There was a problem hiding this comment.
Yes: which is why I suggested you keep this in your local clone without committing it into your Git commits. This should support your refactoring work without me having to look at the changes, and, since I am close to merging this, have to do an extra round trip to ask you to remove it.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
The hook is entirely for my own convenience because I kept pushing changes that were failing tests, it will go once we've got everything else sorted out. To a lesser extent, the example is as well. It mainly works to ensure that my use case remains valid without needing me to screw with my other repos in the meantime, and ultimately will provide a working model after. Just let me know once we're down to these two and I'll take them out last |
|
You could just keep the example in your local workdir uncommitted, similar for the rusty-hook config (though that should really just be a Git pre-push hook probably). Putting stuff in your PR that is not slated to be merged makes reviewing harder. |
|
Git always complains when I do that, there's probably more to its usage that I need to figure out. That, and I'm actually working on this from two different computers, so this also functions as my way to keep things synchronized. |
|
@1Dragoon do you still want to finish this? Would be nice; I think we're going to push out another rc soon and hopefully release 0.17.0 soon after. |
|
I've been out on vacation, I'll get back to it some time next week
…On Wed, May 11, 2022, 2:46 AM Dirkjan Ochtman ***@***.***> wrote:
@1Dragoon </~https://github.com/1Dragoon> do you still want to finish this?
Would be nice; I think we're going to push out another rc soon and
hopefully release 0.17.0 soon after.
—
Reply to this email directly, view it on GitHub
<#420 (comment)>,
or unsubscribe
</~https://github.com/notifications/unsubscribe-auth/ABCBKZVI5NUCBO4M3KVQSBTVJOT2BANCNFSM5RTZ2HCA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Great, thanks! |
|
One thing I'm having difficulty making time for is that making ProgressTracker a single type rather than having the variant type is that I need to refactor all of the example code, but other than that what I have locally works EDIT: Actually maybe there's a different way |
|
I'm a bit stuck. I put a TODO comment in style.rs showing where the problem is. Basically in order to fall within your requirement of having clone and having both stateful and stateless trackers as a single trait. It is nice being able to toss either a simple closure or a more complex tracker type as the same parameter, but I'm not certain how to pull that off given the need for clone to be implemented. Little help anybody? |
|
Sorry, did you mean on line 727? I didn't see an actual |
|
Oh sorry I accidentally committed before adding the todo comment. It's line 25 of style.rs. What I put there is basically just a filler. I just pushed the corrected one with the comment added. EDIT: One other thing I should mention, I had to add an explicit to_string() (name to be bikeshedded) method to the trait as I couldn't work out how to require Display as a type constraint on style.rs line 26 (or if such a thing should even be done at all as it negates the simplicity,) so this was a compromise |
|
Merged in the latest upstream commits and resolved the conflict. One thing though -- I don't really like the default() method name because it's a bit misleading, and it may conflict if the user had a struct that they actually wanted to implement the real default trait for. Just feels a bit needlessly broken to keep it named as such. Not sure what else to call it though, maybe respawn()? |
|
@djc Have you had a chance to review my comments yet? |
|
Uh, I think I already left a round of comments here? I was waiting for you. |
This comment was marked as outdated.
This comment was marked as outdated.
examples/rate_smoothing.rs
Outdated
| @@ -0,0 +1,218 @@ | |||
| #![warn(clippy::pedantic)] | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Oh I figured it out, have to go to the file changes page and then click the review button. They really ought to add some kind of UI hint that you need to do that before anybody can even see your comments, otherwise there's nothing at all to indicate that you have to do so. |
|
BTW I'd still like to be able to tell the built in estimator to noop if we're not using it. |
|
@djc any further concerns with this? |
|
@1Dragoon want me to take a crack at fixing the conflicts? (since they were caused by my most recent commits). I think I can make the changes right in this PR. |
|
Sure, sorry I haven't gotten to this yet because I've been sick. I just needed to do that and get rid of the where clause, but git was kind of being a PITA and I couldn't sit up long enough to deal with it. |
|
I'm feeling a bit better this afternoon so I went ahead and squashed my repo and started to resolve the merge confilcts but this one has me a bit confused: if let Some(formatter) = self.format_map.get(key.as_str()) {
formatter.push_str(&formatter(state).replace('\t', &" ".repeat(self.tab_width)));
}
|
|
I'm not sure that I like what I had to do with line 246, but it works though @djc please review when you can |
It looks OK to me. |
src/style.rs
Outdated
| buf.push_str(&formatter(state).replace('\t', &" ".repeat(self.tab_width))); | ||
| if let Some(tracker) = self.format_map.get(key.as_str()) { | ||
| tracker.write(state, &mut buf); | ||
| buf = buf.replace('\t', &" ".repeat(self.tab_width)); |
There was a problem hiding this comment.
I think we should do a struct TabRewriter(&mut dyn fmt::Write); with an impl fmt::Write that rewrites the tabs before writing them to the inner fmt::Write instead.
Does that seem manageable?
There was a problem hiding this comment.
Not sure I follow. Where would we pass tab_width to that? Or are you suggesting to just use the global default?
|
Great work, thanks for sticking with it! |
|
Sure, no problem. Also not a huge deal, but will it ever show me as a contributor here? Reason I ask is because I am thinking that at some point I might show potential future employers what kind of work I've done, and I'm not sure how I'll be able to do that once I've deleted my forked copy (for cleanliness sake.) |
|
Yes, you should show up as a contributor for this project (or you can send the URL to this PR). |
Very much a janky WIP, and some of the objects I introduced could use better naming though I avoided doing so in the interest of retaining compatibility with existing code. Haven't tested this against older rustc, but as of my current rustc version (latest stable) there are no lints from either regular check or clippy. But let me know if this seems like an ok direction and I'll revise it further.
Somewhen I'll modify that example to show what a more bumpy file transfer looks like, and the advantage of averaging it by the method I used.done