Add dynamic mode to type projector #1858
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #1858 +/- ##
==========================================
- Coverage 50.38% 50.34% -0.05%
==========================================
Files 230 231 +1
Lines 25365 25456 +91
==========================================
+ Hits 12780 12815 +35
- Misses 12585 12641 +56
🚀 New features to boost your workflow:
|
Replace List.nth with List.nth_opt in move_cursor function to safely handle out-of-bounds list access when navigating probe samples. This fixes the Failure(nth) exception that occurred when trying to toggle the type projector modes. Fixes #2097 Co-authored-by: andrew blinn <[email protected]>
| ListUtil.update_nth(idx, map, ((id, entry: Refractors.entry)) => | ||
| ( | ||
| id, | ||
| Refractors.{ | ||
| kind: entry.kind, | ||
| model: new_model, | ||
| }, | ||
| ) | ||
| ), |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let pad_ids = (~settings: Settings.t, n: int, ids: list(Id.t)): list(Id.t) => { | ||
| let len = List.length(ids); | ||
| if (len < n) { | ||
| if (settings.raise_if_padding) { | ||
| raise(Failure("Padding required but not enough ids provided.")); |
There was a problem hiding this comment.
The raise_if_padding setting is added to detect when padding is needed, but the raised exception uses a generic Failure with a string message. Consider defining a specific exception type for padding failures to make error handling more explicit and type-safe.
| let pad_ids = (~settings: Settings.t, n: int, ids: list(Id.t)): list(Id.t) => { | |
| let len = List.length(ids); | |
| if (len < n) { | |
| if (settings.raise_if_padding) { | |
| raise(Failure("Padding required but not enough ids provided.")); | |
| exception PaddingRequired(string); | |
| let pad_ids = (~settings: Settings.t, n: int, ids: list(Id.t)): list(Id.t) => { | |
| let len = List.length(ids); | |
| if (len < n) { | |
| if (settings.raise_if_padding) { | |
| raise(PaddingRequired("Padding required but not enough ids provided.")); |
| let ids = | ||
| current_ids | ||
| @ List.init(needed_ids - List.length(current_ids), _ => Id.mk()); |
There was a problem hiding this comment.
The calculation needed_ids - List.length(current_ids) could be negative if current_ids has more elements than needed_ids. While List.init would fail with a negative value, this should be handled more gracefully. Consider using max(0, needed_ids - List.length(current_ids)) to prevent potential issues.
| let ids = | |
| current_ids | |
| @ List.init(needed_ids - List.length(current_ids), _ => Id.mk()); | |
| let missing_ids = max(0, needed_ids - List.length(current_ids)); | |
| let ids = | |
| current_ids | |
| @ List.init(missing_ids, _ => Id.mk()); |
| let str3 = (id: t) => id |> to_string |> String.sub(_, 0, 3); | ||
| let str8 = (id: t) => id |> to_string |> String.sub(_, 0, 8); | ||
| let cls = (id: t) => "id" ++ str8(id); | ||
|
|
There was a problem hiding this comment.
The helper functions str3, str8, and cls are moved before the pp and show functions. While this is generally fine, it breaks the logical grouping where conversion functions (to_string, of_string, compare) were together, followed by formatting functions (pp, show). This change reduces code organization clarity. Consider keeping related functions together or adding a comment to explain the grouping.
| let qcheck_pads_typ_for_exp_to_segment = | ||
| QCheck.Test.make( | ||
| ~name="No ids are needed to be padded during ExpToSegment", | ||
| ~count=1000, | ||
| QCheck_Util.arb_typ(~minimal_idents=false, 30), | ||
| typ => { | ||
| let padded = PadIds.pad_typ_ids(typ); | ||
| let _ = | ||
| ExpToSegment.typ_to_segment( | ||
| ~settings={ | ||
| secondary: AutoFormat, | ||
| parenthesization: Defensive, | ||
| label_format: QuoteWhenNecessary, | ||
| inline: false, | ||
| fold_case_clauses: false, | ||
| fold_fn_bodies: `NoFold, | ||
| hide_fixpoints: false, | ||
| show_filters: true, | ||
| show_unknown_as_hole: true, | ||
| raise_if_padding: true // Will raise an exception if padding | ||
| }, | ||
| padded, | ||
| ); | ||
| Language.Equality.syntactic.typ(padded, PadIds.pad_typ_ids(padded)); | ||
| }, |
There was a problem hiding this comment.
The test checks that "No ids are needed to be padded during ExpToSegment", but then calls PadIds.pad_typ_ids(padded) at the end, suggesting that padding does occur. The test name and the actual test logic seem inconsistent. If the goal is to verify that padding is idempotent (i.e., padding twice produces the same result), consider renaming the test to better reflect this. Also, the comment on line 22 "Will raise an exception if padding" suggests that padding shouldn't happen, which contradicts the function name and implementation.
|
"Add Statics" -> "Add Type probe" |
|
The hover text on mode is added. I started doing the resizing and abbreviate isn't implemented on types. Plus the sample lengths are a bit ad-hoc for the length state. I can duplicate all the mouse handling but if we're not going to add some sort of a resizable component this is going to be a maintenance burden. @disconcision if you want to see if our 🤖 overlords can make the type probe resizable that would be great. |
2 participants
Adds a dynamic mode to Type projector
and Probes on typesType projector with dynamic type
Removed
Probes on types
Allows you to add probes and dyntype probes on types. This works by adding a
Probetype similar to the one for patterns and expressions. TheType"observes" any expression that flows through it in an ascription e.g."Hello" : Probe(?)will add "Hello" to the probe. This also allows for projectors on type to havedynamicsenabled as they will get dynamic information during evaluation.Known issues