-
Notifications
You must be signed in to change notification settings - Fork 26
Support for renaming JS methods when re-exporting #1010
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the methodParams field is it possible to encode the idea of sum type? I.e require a network magic number or default to mainnet?
@Jimbo4350, the easiest we could do is have a nullable |
| { methodName :: String | ||
| -- ^ Name of the method in the virtual object of the JS API (which should match the exported function). | ||
| -- ^ Name of the global method name in the API (which should match the exported function and it must be unique globally). | ||
| , methodSimpleName :: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| , methodSimpleName :: String | |
| , methodSimpleName :: Maybe String |
I think it should default to methodName. I'd throw
instance IsString s => IsString (Maybe s) where
fromString = Just . fromStringinto the mix with OverloadedStrings. I think we tried that in the past, but it was too magical to the tweag folks.
| , MethodInfoEntry $ | ||
| MethodInfo | ||
| { methodName = "estimateMinFee" | ||
| , methodSimpleName = "estimateMinFee" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those duplicated fields look like a good use case for some kind of smart constructor. But then you have unnamed fields and it would be nice to undo that with something like named but then I guess it turns into a new can of worms. 😄
Changelog
Context
See #1003 (comment). Names of functions exported by the API need to be unique, but we can rename them as we re-export them for the JS API function.
How to trust this PR
The tests passing gives strong guarantee the renaming is done correctly. Other than that, it could be that there is some file that wasn't updated that we missed. And also it would be a matter of checking whether the comments and code design is sensible.
I would look at the commit separately too, because the first one is a refactoring, and the second one is a simple change.
Checklist