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

Linter fix #339

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Linter fix #339

wants to merge 25 commits into from

Conversation

Bus42
Copy link

@Bus42 Bus42 commented May 26, 2022

Linting Errors Fix

Several linting errors are reported when running the backend locally. This PR addresses those errors by adding type definitions of : any

  • Assign type of : any to functions flagged by the linter

I only made the fixes necessary to address the linting errors
https://www.loom.com/share/d9f694621868415a9f58e4f7c8be8531

Changes to package.json and package.lock.json have been reverted since Loom recording

Bus42 added 5 commits May 25, 2022 12:34
Replaces occurences of 'catch (err)' with 'catch (err: any)'
Replaces occurences of 'catch (e)' with 'catch (e: any)'
Replaces occurences of 'catch(err)' with 'catch(err: any)'
Replaces let result; with let result:any
Replaces '--theme minimal' with '--theme default' in package.json
@Bus42 Bus42 marked this pull request as ready for review May 26, 2022 18:06
Copy link
Contributor

@TashaSkyUp TashaSkyUp left a comment

Choose a reason for hiding this comment

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

The changes to package-lock.json and package.json should be explained or removed. These files are very critical to maintain correctly. If you have tested the changes to these files to be working and advantageous please make another PR for them.

@TashaSkyUp
Copy link
Contributor

The changes to package-lock.json and package.json should be explained or removed. These files are very critical to maintain correctly. If you have tested the changes to these files to be working and advantageous please make another PR for them.

I see you did mention packages.json. it's just wild that it caused that many changes in the lock file. I will test this on windows, @Bus42 can you verify its working on linux? and I think @rmjuarez12 can verify mac? I forget are you on mac?

@Bus42
Copy link
Author

Bus42 commented May 26, 2022

I can verify that it works in Ubuntu. I did explain the change to package.json in the Loom, but I will update the PR text with the explanation as well.

@Bus42
Copy link
Author

Bus42 commented May 26, 2022

I realized that the changes to package.json were due to errors when running npm run deploy which I have been informed is no longer used and reverted the changes to both files.

@TashaSkyUp
Copy link
Contributor

b71b03e reverts the package files. I used this command that you might find useful!

git restore --source main~7 package-lock.json

@TashaSkyUp
Copy link
Contributor

I'm not going to be able to test this on windows until tomorrow.

@Bus42
Copy link
Author

Bus42 commented May 27, 2022

I've added explicit types to all instances with the exception of

  • /api/middleware/oktaRequire.ts
  • /api/words/utils.ts

These are proving a bit tricky

@Bus42
Copy link
Author

Bus42 commented May 31, 2022

I've addressed the typing in the remaining two files (oktaRequire.ts and utils.ts) by creating interfaces for both files and a constructor in oktaRequire.ts.

Express functions still need explicit typing. They are currently all typed : any to suppress the errors and allow build, but this is more of a "band-aid" fix. To assign types properly, I will need to install the @types/express library.

@Bus42 Bus42 requested a review from jlong5795 May 31, 2022 19:23
This should not have been touched as the error is unrelated
@Bus42 Bus42 requested a review from TashaSkyUp May 31, 2022 19:27
Copy link
Contributor

@TashaSkyUp TashaSkyUp left a comment

Choose a reason for hiding this comment

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

Hand testing passed on windows. Thank you @Bus42 !

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.

None yet

2 participants