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: change note type to string #622

Merged
merged 5 commits into from
Oct 13, 2021
Merged

fix: change note type to string #622

merged 5 commits into from
Oct 13, 2021

Conversation

kevindavee
Copy link
Contributor

@kevindavee kevindavee commented Oct 11, 2021

Description of the changes

note should be type string. Changing the test and the Typescript type.

@coveralls
Copy link

coveralls commented Oct 11, 2021

Coverage Status

Coverage remained the same at 88.829% when pulling 99ff596 on fix/note-type into 20dfa5a on master.

Copy link
Member

@yomarion yomarion left a comment

Choose a reason for hiding this comment

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

I don't really understand why note would not be string anymore, it's specified as a string.

@kevindavee
Copy link
Contributor Author

Based on the test, it looks like note accepts anything.

I'm not sure which is the source of truth but this test has been there for 3 years based on the git commit @yomarion

@kevindavee kevindavee changed the title fix: change type to unknown fix: change note type to string Oct 11, 2021
Copy link
Contributor

@vrolland vrolland left a comment

Choose a reason for hiding this comment

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

I think any is better because declarative's actions are now very generic.
Only string is too restrictive. For example in our case of manual payment declaration on a crypto request:

{ transactionHash: '0x12345' }

is more explicit than only "0x12345".

@vrolland
Copy link
Contributor

vrolland commented Oct 11, 2021

I don't really understand why note would not be string anymore, it's specified as a string.

@yomarion @kevindavee
note is a string in the request-client.js and any in advanced-logic.
I think we should modify the request-clients.js to be more generic and reflect the change we made in advanced logic.

@yomarion
Copy link
Member

I don't really understand why note would not be string anymore, it's specified as a string.

@yomarion @kevindavee note is a string in the request-client.js and any in advanced-logic. I think we should modify the request-clients.js to be more generic and reflect the change we made in advanced logic.

It's defined here as a string. What is it if it's not a string? It should be defined clearly before changing the whole implementation.

@kevindavee
Copy link
Contributor Author

I don't know the historical why there's a discrepancy between the types in advance-logic and request-client.js. But, in my opinion, note sounds like a string instead of any.

@kevindavee
Copy link
Contributor Author

@vrolland can you review this PR again as we have reached a consensus to have note as string?

@kevindavee kevindavee merged commit 4cc80d7 into master Oct 13, 2021
@kevindavee kevindavee deleted the fix/note-type branch October 13, 2021 08:29
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

4 participants