StudioInfinity/dyna3
2
0
Fork 1
Code Issues 44 Pull requests Projects 2 Releases 2 Wiki Activity Actions

Generalize constraints to observables #48

Merged
glen merged 25 commits from Vectornaut/dyna3:observables_on_main into main 2025-03-10 23:43:25 +00:00
Conversation 69 Commits 25 Files changed 7 +11 -10
2 changed files with 11 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 894931a6e7 - Show all commits

12
app-proto/src/assembly.rs
View file

@ -181,13 +181,11 @@ pub struct Regulator {
}
impl Regulator {
pub fn try_set(&self, set_pt_spec: String) -> bool {
match SpecifiedValue::try_from(set_pt_spec) {
Ok(set_pt) => {
self.set_point.set(set_pt);
true
}
Err(_) => false
/* TO DO */
// if it's called for, add a `set` method that takes a bare SpecifiedValue
pub fn set_if_ok<E>(&self, set_pt_result: Result<SpecifiedValue, E>) {
if let Ok(set_pt) = set_pt_result {
self.set_point.set(set_pt);
}
}
}

9
app-proto/src/outline.rs
View file

@ -13,6 +13,7 @@ use crate::{
ElementKey,
glen marked this conversation as resolved Outdated
glen commented 2025-03-08 00:26:29 +00:00
Outdated
Review
Copy link

Another use that needs to be harmonized with all our uses

Another `use` that needs to be harmonized with all our `uses`
Vectornaut commented 2025-03-08 23:31:30 +00:00
Outdated
Review
Copy link

Corrected, as discussed above.

Corrected, as discussed [above](#issuecomment-2317).
Regulator,
RegulatorKey,
SpecifiedValue,
SpecifiedValue::*
}
};
@ -55,9 +56,11 @@ fn RegulatorInput(regulator: Regulator) -> View {
},
placeholder=regulator.measurement.with(|result| result.to_string()),
bind:value=value,
on:change=move |_| valid.set(
regulator.try_set(value.get_clone_untracked())
),
on:change=move |_| {
let set_pt_result = SpecifiedValue::try_from(value.get_clone_untracked());
valid.set(set_pt_result.is_ok());
glen marked this conversation as resolved Outdated
glen commented 2025-03-04 16:00:16 +00:00
Outdated
Review
Copy link

This looks like the code is checking the OK-ness of the try_from twice. Is there a concise clear way to bind a variable to the SpecifiedValue payload in the OK return case and set valid to true and call the as-yet-unwritten regulator.set with that specified value, in that case, and in the alternative (Error) case, just set valid to false and do nothing to the regulator? That organization would seem to read more crisply, as long as it isn't too cumbersome to write... And in fact, if I recall, isn't that sort of thing what if let OK(blah) = try_from { ... } else {valid.set(false)} is for?

P.S. If the code here switched to something like this suggestion, then as this is the only instance of set_if_ok, you could remove that method in favor of a simpler set method, or in fact maybe just a direct call of regulator.set_point.set(...), eliminating the entire impl Regulator block at lines 127-135 of assembly.rs, which seems like a win.

So I guess I am specifically suggesting replacing this block with

if let Ok(spec) = SpecifiedValue::try_from(value.get_clone_untracked()) {
   valid.set(true)
   regulator.set_point.set(spec)
} else { valid.set(false) }

-- presuming the syntax is OK -- and just removing that impl Regulator block in assembly.rs altogether.

This looks like the code is checking the OK-ness of the try_from twice. Is there a concise clear way to bind a variable to the SpecifiedValue payload in the OK return case and set `valid` to true and call the as-yet-unwritten regulator.set with that specified value, in that case, and in the alternative (Error) case, just set `valid` to false and do nothing to the regulator? That organization would seem to read more crisply, as long as it isn't too cumbersome to write... And in fact, if I recall, isn't that sort of thing what `if let OK(blah) = try_from { ... } else {valid.set(false)}` is for? P.S. If the code here switched to something like this suggestion, then as this is the only instance of `set_if_ok`, you could remove that method in favor of a simpler `set` method, or in fact maybe just a direct call of `regulator.set_point.set(...)`, eliminating the entire `impl Regulator` block at lines 127-135 of assembly.rs, which seems like a win. So I guess I am specifically suggesting replacing this block with ``` if let Ok(spec) = SpecifiedValue::try_from(value.get_clone_untracked()) { valid.set(true) regulator.set_point.set(spec) } else { valid.set(false) } ``` -- presuming the syntax is OK -- and just removing that `impl Regulator` block in assembly.rs altogether.
Vectornaut commented 2025-03-05 22:25:09 +00:00
Outdated
Review
Copy link

So I guess I am specifically suggesting replacing this block with

if let Ok(spec) = SpecifiedValue::try_from(value.get_clone_untracked()) {
    valid.set(true)
    regulator.set_point.set(spec)
} else { valid.set(false) }

That sounds great to me. I'd reorganize the block to avoid the repetition of valid.set. Depending on whether you prefer if let or match, we could do either of these:

valid.set(
    if let Ok(set_pt) = SpecifiedValue::try_from(value.get_clone_untracked()) {
        regulator.set_point.set(set_pt);
        true
    } else { false }
)

valid.set(
    match SpecifiedValue::try_from(value.get_clone_untracked()) {
        Ok(set_pt) => {
            regulator.set_point.set(set_pt);
            true
        }
        Err(_) => false
    }
)

In the match version, the argument of valid.set is just an inlined version of the old Regulator::try_set method. If we want to go this route, and you don't have a strong preference for inlining, I'd recommend just reverting commit 894931a, which replaced Regulator::try_set with Regulator::set_if_ok. We can also rewrite try_set to use if let instead of match, of course.

> So I guess I am specifically suggesting replacing this block with > > ```rust > if let Ok(spec) = SpecifiedValue::try_from(value.get_clone_untracked()) { > valid.set(true) > regulator.set_point.set(spec) > } else { valid.set(false) } That sounds great to me. I'd reorganize the block to avoid the repetition of `valid.set`. Depending on whether you prefer `if let` or `match`, we could do either of these: ```rust valid.set( if let Ok(set_pt) = SpecifiedValue::try_from(value.get_clone_untracked()) { regulator.set_point.set(set_pt); true } else { false } ) ``` ```rust valid.set( match SpecifiedValue::try_from(value.get_clone_untracked()) { Ok(set_pt) => { regulator.set_point.set(set_pt); true } Err(_) => false } ) ``` In the `match` version, the argument of `valid.set` is just an inlined version of the old `Regulator::try_set` method. If we want to go this route, and you don't have a strong preference for inlining, I'd recommend just reverting commit 894931a, which replaced `Regulator::try_set` with `Regulator::set_if_ok`. We can also rewrite `try_set` to use `if let` instead of `match`, of course.
regulator.set_if_ok(set_pt_result);
},
on:keydown={
move |event: KeyboardEvent| {
match event.key().as_str() {