Renames for readability #409
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
swansontec
requested changes
Jul 25, 2024
Comment on lines
1005
to
1008
| common: CommonParams, | ||
| needsTxSpecific: boolean, | ||
| serverUri: string, | ||
| txId: string |
There was a problem hiding this comment.
So here we have positional parameters, but above updateTransactionsSpecific has named parameters. I'm not sure if we want to be more consistent about named / positional parameters, but I do appreciate that CommonParams is cleaned up now.
This is just an observation. There are no changes needed unless you want to.
swansontec
approved these changes
Jul 25, 2024
The processor is a DAL, so it's better to be named appropriately. All the type names will be suffixed with "Data" to distinguish them from other models of the same kind of object (e.g. TransactionData vs EdgeTransaction).
This component of the plugin makes more sense to be name "processor" because it's the component that processes data from the network and storage for the engine.
This adds documentation comments to each processor function to briefly describe the function's intent.
Trezor is known to rate limit. In the future, a mock Blockbook interface is the correct fix.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
none