Remove internal is_visible field from ComputedTask#480
Conversation
The `is_visible` field is an internal logic detail and should be hidden from the view layer. It has been removed from `ComputedTask` struct and `get_prioritized_tasks` function. Compliance tests have been updated to ignore this field in assertions while maintaining JSON compatibility. Co-authored-by: matta <37314+matta@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Merging to
|
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
@jules, PR rejected as is. Please investigate ==> We need to retain a way to validate these values in the compliance.rs test harness. This is vital for debugging the algorithm code. Look for a way to do this while refraining from exposing these "incidental" computed values to the UI layer. The solution may involve introducing multiple types.
The `is_visible` field is an internal logic detail and has been removed from the `ComputedTask` view model. However, to ensure the correctness of the visibility logic, a new `ComputedTaskDebug` struct (which includes `is_visible`) has been introduced. The compliance test suite has been updated to use `get_prioritized_tasks_debug` and assert against `ComputedTaskDebug`, preserving the original validation capabilities without polluting the UI layer. Co-authored-by: matta <37314+matta@users.noreply.github.com>
|
Seeing this, I liked the original approach better, where ComputedTasksDebug didn't exist and is_visible as simply removed from it. Let's go back to that version, but also remove support for is_visible from the compliance fixtures, and update all the fixtures. |
1 participant
Removed
is_visiblefield fromComputedTaskstruct incrates/tasklens-core/src/types/transient.rsand stopped its initialization incrates/tasklens-core/src/domain/priority.rs. Updatedcrates/tasklens-core/tests/compliance.rsto rename the expected property to_is_visible(with#[serde(rename = "is_visible")]) to maintain compatibility with existing JSON fixtures, and removed the assertion logic for this field. Verified that tests pass withcargo test --test complianceandcargo check --workspace.PR created automatically by Jules for task 14835695415961833927 started by @matta