Skip to content

Conversation

@philipjackson
Copy link
Contributor

Description

This was necessary for the makeAnnotationsJson unit test in DOMINATE, because it turns out the Annotation interface doesn't actually allow for null coordinates for bounding boxes (which is currently how "no bounding box" is represented) when strictNullChecks is enabled. We have strictNullChecks set in DOMINATE but not in ANNOTATE, which is why this didn't cause a problem there.

Also exported the Spline and BoundingBox interfaces so I don't have to duplicate them in the unit test.

Checklist:

Put an x in the boxes that apply to this pull request (you can also fill these out after opening the pull request). If you're unsure about any of these, don't hesitate to leave a comment on this pull request!

  • I have read the gliff.ai Contribution Guide.
  • I have requested to pull a branch and not from main.
  • I have checked all commit message styles match the requested structure.
  • My code follows the style guidelines of this project.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have performed a self-review of my own code.
  • I have assigned 3 or less reviewers.
  • New and existing unit tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • My changes generate no new warnings.
  • I have made corresponding changes to the documentation.
  • New database changes have been committed.
  • If appropriate, I have bumped any version numbers.

@philipjackson philipjackson changed the title Export spline bounding box fix: explicitly allow null coords in BoundingBox interface Feb 25, 2022
@gliff-ai-robot gliff-ai-robot added the bug [Problem] Something isn’t working or looking right. label Feb 25, 2022
Copy link
Member

@ChasNelson1990 ChasNelson1990 left a comment

Choose a reason for hiding this comment

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

Should we also turn strictNullCheck on for ANNOTATE?

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-wave-0f4ddd903-673.westeurope.azurestaticapps.net

@philipjackson
Copy link
Contributor Author

Should we also turn strictNullCheck on for ANNOTATE?

Don't know. I think it was Silvia who enabled them originally, there was a specific reason but I've forgotten it. I think it's generally regarded as good practice, but will probably require us to go through the repo and put | null everywhere.

@ChasNelson1990
Copy link
Member

@cooper667 @SilviaZeta thoughts?

@cooper667
Copy link
Contributor

@cooper667 @SilviaZeta thoughts?

Yea, we should have one tsconfig we extend everywhere same as eslint config

@SilviaZeta
Copy link
Contributor

@cooper667 @SilviaZeta thoughts?

yes, I needed the setting for schema validation. It has proven to be really useful, but as @philipjackson says, it will require adding null wherever this is missing and to sort out any type inconsistencies to do with it.

@philipjackson
Copy link
Contributor Author

Enabling strictNullChecks in ANNOTATE created 198 build errors, as well as some lint errors that don't even seem related to null checks, strangely. If we're enabling strictNullChecks then I think it should be a separate PR, and in the likely event of unforeseen headaches it could turn into a full day's work, so I suggest we put a pin in it for now and merge the PR

@ChasNelson1990 ChasNelson1990 self-requested a review February 28, 2022 14:03
@philipjackson philipjackson merged commit 73316e0 into main Feb 28, 2022
@philipjackson philipjackson deleted the export-spline-boundingBox branch February 28, 2022 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug [Problem] Something isn’t working or looking right.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants