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

fix: part 2 of lint warnings resolved for batch 7/26 #1356 : Commit to be reviewed #2098

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

akankshadixit
Copy link

@akankshadixit akankshadixit commented Jun 24, 2022

style(common): fix batch 7/26 of linter warnings

Changes made:

  • Fixed lint warnings associated with any-type for truthyness and nonBlankString checking
  • Fixed lint warnings associated with type-aliases in logger
  • Fixed lint warnings associated with non-any type in getAllMethodNames and getAllFieldNames
  • Fixed lint errors associated with unexpected any for private-public keypairs
  • Added an isRecord user-defined type guard in the common package that
    can narrow unknown types into Record<string, unknown>

Fixes #1356

Co-authored-by: Peter Somogyvari [email protected]

Signed-off-by: akankshadixit [email protected]
Signed-off-by: Peter Somogyvari [email protected]

Copy link
Contributor

@jagpreetsinghsasan jagpreetsinghsasan 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 looks fine, just a squash merge of the commits required.

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@akankshadixit LGTM, thank you!

Some advice on the admin chores to speed things up next time: If a reviewer requested changes and then you addressed them (like you did here) then it helps if you then hit the little circular arrows button next to their name (in this case Jagpreet) which is called the "re-request review" button which will send them a ping to come back and check on the changes you've made in response to their change request. Without this it doesn't actually pop up on their feed as the PR needing review again.

image

@akankshadixit
Copy link
Author

Hi @petermetz,

Thank you. I will ensure this in future.

@petermetz
Copy link
Contributor

Hi @petermetz,

Thank you. I will ensure this in future.

@akankshadixit No worries at all, it's not a big deal either way, I'm just letting you know for future reference. :-)

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@akankshadixit The build seems to be broken now, please double check if it works locally and then let's go from there.

I'm not sure if you have access to the CI Logs or not but the build-dev check failed with this:

5:41:00 AM - Project 'packages/cactus-common/tsconfig.json' is out of date because output file 'packages/cactus-common/dist/lib/main/typescript/bools.js' does not exist
5:41:00 AM - Building project '/home/runner/work/cactus/cactus/packages/cactus-common/tsconfig.json'...
Error: packages/cactus-common/src/test/typescript/unit/objects/get-all-method-names.test.ts(8,49): error TS2345: Argument of type 'A' is not assignable to parameter of type 'Record<string, unknown>'.
  Index signature is missing in type 'A'.
5:41:04 AM - Project 'packages/cactus-core-api/tsconfig.json' can't be built because its dependency 'packages/cactus-common/tsconfig.json' has errors

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

^^

@akankshadixit akankshadixit force-pushed the Part2batch7/26 branch 2 times, most recently from 8a62776 to c05e74d Compare July 27, 2022 17:15
@akankshadixit akankshadixit requested a review from petermetz July 27, 2022 17:16
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@akankshadixit LGTM now with the additions I made just now it does not have to be a breaking change and we are good to go!

@petermetz petermetz enabled auto-merge (rebase) June 29, 2023 07:27
Changes made:
- Fixed lint warnings associated with any-type for truthyness and nonBlankString checking
- Fixed lint warnings associated with type-aliases in logger
- Fixed lint warnings associated with  non-any type in getAllMethodNames and getAllFieldNames
- Fixed lint errors associated with unexpected any for private-public keypairs
- Added an isRecord user-defined type guard in the common package that
can narrow `unknown` types into `Record<string, unknown>`

Fixes hyperledger-cacti#1356

Co-authored-by: Peter Somogyvari <[email protected]>

Signed-off-by: akankshadixit <[email protected]>
Signed-off-by: Peter Somogyvari <[email protected]>
@petermetz petermetz merged commit 166ab4f into hyperledger-cacti:main Jun 29, 2023
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.

style: 2021-09-20 linter warnings batch 7 / 26
4 participants