Skip to content

Convert type any to type unknown#4508

Merged
tomholub merged 23 commits intomasterfrom
issue-4490-convert-type-any-to-type-unknown
Jul 14, 2022
Merged

Convert type any to type unknown#4508
tomholub merged 23 commits intomasterfrom
issue-4490-convert-type-any-to-type-unknown

Conversation

@martgil
Copy link
Collaborator

@martgil martgil commented Jun 24, 2022

This PR's goal is to convert type any to type unknown.

To check the warnings on master branch:
npm run test_eslint

close #4490 // if this PR closes an issue

issue #0000 // if it doesn't close the issue yet


Tests (delete all except exactly one):

  • Does not need tests (refactor only, docs or internal changes)
  • Difficult to test (explain why)
  • Not worth testing
  • Tests will be added later (issue #...)
  • Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@martgil martgil requested a review from rrrooommmaaa as a code owner June 24, 2022 09:58
@martgil martgil marked this pull request as draft June 24, 2022 09:58
@martgil
Copy link
Collaborator Author

martgil commented Jun 24, 2022

Hello sir, @ioanmo226 - I initially started with converting 5 of these... there is some type of checking in common places like catching an error... if this looks okay maybe I can start converting type any to unknown from there?

@tomholub
Copy link
Collaborator

@martgil please update this PR to make it ready for reviews and merging

Comment on lines 1 to 12
import * as forge from 'node-forge';

export as namespace SquireClass;

declare global {
interface Window {
// @ts-ignore
Squire: SquireClass;
forge: typeof forge;
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also this is wrong to add forge into squire definitions. Let's remove it and file an issue for Ioan to have a look later.

Copy link
Collaborator Author

@martgil martgil Jul 14, 2022

Choose a reason for hiding this comment

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

okay sir, I'll remove it - I just converted most of it from any to unknown but there are still some that need to have proper types but I can't convert them just yet. I can't figure out what to do with them just yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's ok. You can leave these for another PR in the future.

@martgil martgil marked this pull request as ready for review July 14, 2022 09:15
Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

This is definitely an improvement. @ioanmo226 if there is still any work to do on this issue, you may look into it in a separate PR. Thanks!

console.error(result.supplementaryOperationsErrors);
Catch.setHandledTimeout(() => {
Ui.toast(result.supplementaryOperationsErrors[0]); // tslint:disable-line:no-unsafe-any
Ui.toast(result.supplementaryOperationsErrors[0] as string);
Copy link
Collaborator

Choose a reason for hiding this comment

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

as X is not so safe, it would be safer to test for what type it is and treat it appropriately

@tomholub tomholub merged commit 09a7e84 into master Jul 14, 2022
@tomholub tomholub deleted the issue-4490-convert-type-any-to-type-unknown branch July 14, 2022 13:21
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.

no-explicit-any warnings - prefer unknown

2 participants