Skip to content

Conversation

@PabstMirror
Copy link
Collaborator

  • Shows detailed expected for arrays (only when array is sole type)

before:

1 │ drawLine3D [getPosASL player, getPosASL cursorTarget, "red"];
  │ ^^^^^^^^^^ expected Array<PosAGL>, Array
  = note: found type was Array

now:

  │ ^^^^^^^^^^ expected Array[(Array<PosAGL>), (Array<PosAGL>), (Array)]
  = note: found type was Array[(Array<PosASL>), (Array<PosASL>), (String)]
  • Only show single error for problem-side on binary commands
aVic setFuel true; // only error on RHS

before (only reported LHS):

1 │ aControl drawIcon ["iconMan", aColor, position player, 1, "big"];
  │ ^^^^^^^^ expected Control
  = note: found type was Anything

now:

1 │ aControl drawIcon ["iconMan", aColor, position player, 1, "big"];
  │                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected Array[(String), (Array), (Anything), (Number), (Number)]
  = note: found type was Array[(String), (Anything), (Array<PosAGL>), (Number), (String)]

partially from #1136

@codecov
Copy link

codecov bot commented Dec 2, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.2%. Comparing base (784978f) to head (fcf939d).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
libs/sqf/src/analyze/inspector/game_value.rs 72.5% 25 Missing ⚠️
libs/sqf/src/analyze/inspector/issue.rs 75.0% 15 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
libs/sqf/src/analyze/inspector/commands.rs 84.6% <100.0%> (ø)
libs/sqf/src/analyze/inspector/mod.rs 97.2% <100.0%> (+0.3%) ⬆️
libs/sqf/src/analyze/lints/s12_invalid_args.rs 73.3% <100.0%> (ø)
libs/sqf/src/analyze/inspector/issue.rs 75.0% <75.0%> (ø)
libs/sqf/src/analyze/inspector/game_value.rs 76.6% <72.5%> (-2.6%) ⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@PabstMirror PabstMirror marked this pull request as ready for review December 2, 2025 19:01
@BrettMayson
Copy link
Owner

BrettMayson commented Dec 5, 2025

1 │ aControl drawIcon ["iconMan", aColor, position player, 1, "big"];
  │                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected Array[(String), (Array), (Anything), (Number), (Number)]
  = note: found type was Array[(String), (Anything), (Array<PosAGL>), (Number), (String)]

I'm worried this'll easily get too long and look awful in terminals

I think it'd be better to have the label message be just incorrect value or something short, and have the note contain both, like

= note: found    `Array[(String), (Anything), (Array<PosAGL>), (Number), (String)]`
        expected `Array[(String), (Array), (Anything), (Number), (Number)]`

@PabstMirror
Copy link
Collaborator Author

Was also thinking we could shorten some
like boolean->bool, Anything->Any, Nothing->Nil ?
for arrays could maybe use 3char String->Str Number->Num but I don't really like cutting it off that much

@BrettMayson
Copy link
Owner

I think shortening them like that could be good too, everyone would understand Bool, Str, Num, Any, Nil

I think doing that and moving it to the not would be pretty good, would have to be something really long
to wrap on a standard terminal

@PabstMirror
Copy link
Collaborator Author

1 │  aControl drawIcon ["iconMan", aColor, position player, 1, "big"];
  │                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type not expected
  │
  = note: found    Array[(String), (Any), (Array<PosAGL>), (Number), (String)]
          expected Array[(String), (Array), (Any), (Number), (Number)]

Shorting things to 3 chars would make things "monospaced"
but I also think it hurts readability in most cases
and I don't think there is a good way to put "structured text" into 3 chars
For arrays the mismatch will always be the last element as it stops on first error

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves the inspector's error reporting for invalid arguments on binary commands and arrays. The main enhancement is showing detailed expected types for arrays (when array is the sole expected type) and only reporting errors for the problematic side of binary commands instead of reporting errors for both sides.

Key Changes

  • Refactored InvalidArgs and Issue enums into a separate issue.rs module with enhanced error message formatting
  • Modified binary command validation to track which sides matched and only report errors for mismatched sides
  • Enhanced array type error messages to show detailed element-by-element type expectations
  • Changed default field in DefaultDifferentType from Range<usize> to Option<Range<usize>>

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
libs/sqf/src/analyze/inspector/issue.rs New module containing refactored Issue and InvalidArgs enums with improved error message formatting methods
libs/sqf/src/analyze/inspector/game_value.rs Added tracking for matched syntaxes, enhanced from_wiki_value to return sets, added vec_to_string for detailed array formatting, and updated type display names
libs/sqf/src/analyze/inspector/mod.rs Refactored to import from issue.rs and modified binary command validation to only report errors for non-matching sides
libs/sqf/src/analyze/inspector/commands.rs Updated DefaultDifferentType construction to wrap default range in Some()
libs/sqf/src/analyze/lints/s12_invalid_args.rs Updated pattern matching to handle Some(default) for the optional default field
libs/sqf/tests/snapshots/*.snap Updated test snapshots reflecting improved error messages and reduced error count for binary commands

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@BrettMayson BrettMayson merged commit 6b10275 into main Dec 17, 2025
40 checks passed
@BrettMayson BrettMayson deleted the argA branch December 17, 2025 02:37
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.

3 participants