Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address @ts-expect-error #139

Open
pixelzoom opened this issue Feb 27, 2023 · 8 comments
Open

Address @ts-expect-error #139

pixelzoom opened this issue Feb 27, 2023 · 8 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 27, 2023

Address occurrences of @ts-expect-error.

Then make this change in package.json:

-              "ts-expect-error": "allow-with-description",
+              "ts-expect-error": true
@pixelzoom pixelzoom self-assigned this Feb 27, 2023
@pixelzoom
Copy link
Contributor Author

Unassigning until we're working on this sim.

@pixelzoom pixelzoom removed their assignment Mar 8, 2023
pixelzoom added a commit that referenced this issue Apr 18, 2023
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 18, 2023

As of this writing, all files have been converted to TypeScript.

There are 17 occurrences of @ts-expect-error, all in view code, and all related to the same problem:

In Challenge.ts, we have guessProperty: Property<Line | NotALine>, because the user's guess (which can consist of up to 3 points) may not be a valid line. This causes problems with the many places where we are expecting Property<Line>. Note that the NotALine case occurs only in the "Place The Points" challenge type, where the user must place 3 points on the graph.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 25, 2023

There are now 14 occurrences of @ts-expect-error, all in graphing-lines/js/linegame/view/, and all with this description:

// @ts-expect-error guessProperty is Property<Line | NotALine>
screenshot_2667

PlaceThePoints is the only challenge where NotALine is relevant. In PlaceThePoints.ts, line 64:

    // Update the guess when the points change.
    // unmultilink unnecessary because PlaceThePoints owns these Properties.
    Multilink.multilink(
      [ this.p1Property, this.p2Property, this.p3Property ],
      ( p1, p2, p3 ) => {
        const line = new Line( p1.x, p1.y, p2.x, p2.y, LineGameConstants.GUESS_COLOR );
        if ( line.onLinePoint( p3 ) ) {
          // all 3 points are on a line
          this.guessProperty.value = line;
        }
        else {
          // the 3 points don't form a line
64        this.guessProperty.value = new NotALine();
        }
      } );

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 11, 2023

In the above commits, I added this cast to address challenge.guessProperty:

assert && assert( challenge.guessProperty.value instanceof Line );
const guessProperty = challenge.guessProperty as Property<Line>;

Then I realized that this is unsafe, because the cast is only valid for the initial value of guessProperty. So I reverted.

There are also some Color | string => TColor changes in the above commits that are valid, and have been kept.

I also made LineNode take lineProperty: Property<Line> | Property<Line | NotALine>, so we're down to 12 uses of @ts-expect-error.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 11, 2023

In the above commit, I replaced a few more unsafe as casts with @ts-expect-error.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 11, 2023

In the above commits, I simplified lineProperty: Property<Line> | Property<Line | NotALine> to lineProperty: Property<Line | NotALine> in the above commits.

@pixelzoom
Copy link
Contributor Author

Unassigning until work on this sim resumes.

@pixelzoom pixelzoom removed their assignment Dec 11, 2023
@pixelzoom
Copy link
Contributor Author

This does not need to be completed for the 1.4 release, #142.

pixelzoom added a commit that referenced this issue Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant