Skip to content

fix: update decoded data param types #59

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 22, 2022
Merged

fix: update decoded data param types #59

merged 2 commits into from
Feb 22, 2022

Conversation

@DiogoSoaress
Copy link
Contributor

@DiogoSoaress DiogoSoaress commented Feb 22, 2022 •
edited
Loading

Update decoded-data endpoint return types.

From https://gnosis.github.io/safe-client-gateway/docs/common/models/data_decoded/enum.ParamValue.html , the params value can be contain nested string[]

@github-actions
Copy link

github-actions bot commented Feb 22, 2022 •
edited
Loading

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total
Report generated by eslint-plus-action

katspaugh
katspaugh reviewed Feb 22, 2022
View reviewed changes
src/types/decoded-data.ts Outdated
name: string
type: string
value: string
value: string | ParamValue[]
Copy link
Member

Choose a reason for hiding this comment

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

value: ParamValue perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't that type leave out the nested arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied your suggestion after changing the type ParamValue = (string | ParamValue)[] as per @iamacook suggestion

iamacook
iamacook reviewed Feb 22, 2022
View reviewed changes
src/types/decoded-data.ts Outdated
data: string
}

type ParamValue = string | string[]
Copy link
Contributor

@iamacook iamacook Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If you change this to reference itself, you can use it as value without needing arrays of arrays.

Suggested change
type ParamValue = string | string[]
type ParamValue = string | (string | ParamValue)[]

@DiogoSoaress DiogoSoaress force-pushed the update-decoded-data-param-types branch from 3f6a32d to 9edc6a8 Compare February 22, 2022 12:02
iamacook
iamacook approved these changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

Looks good now!

katspaugh
katspaugh reviewed Feb 22, 2022
View reviewed changes
src/types/decoded-data.ts Outdated
data: string
}

type ParamValue = string | (string | ParamValue)[]
Copy link
Member

Choose a reason for hiding this comment

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

I think you don't need the second string.
type ParamValue = string | ParamValue[] is already recursive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch
image

Copy link
Contributor Author

@DiogoSoaress DiogoSoaress Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah that's right. Just changed it

@DiogoSoaress DiogoSoaress force-pushed the update-decoded-data-param-types branch from 9edc6a8 to b70cb5c Compare February 22, 2022 13:45
@DiogoSoaress DiogoSoaress requested review from katspaugh and removed request for iamacook February 22, 2022 15:59
katspaugh
katspaugh approved these changes Feb 22, 2022
View reviewed changes
Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

👍

@katspaugh
Copy link
Member

Please also bump the version in package.json if you want to release it on npm.

@DiogoSoaress
Copy link
Contributor Author

Bumped the version in package.json 👍 to create a release

@DiogoSoaress DiogoSoaress merged commit eccf626 into main Feb 22, 2022
@DiogoSoaress DiogoSoaress deleted the update-decoded-data-param-types branch February 22, 2022 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@katspaugh katspaugh katspaugh approved these changes

+1 more reviewer

@iamacook iamacook iamacook approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@DiogoSoaress @katspaugh @iamacook