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.
Add sdk feature into feat 2.0 #106
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
Add sdk feature into feat 2.0 #106
Changes from all commits
1d5778d
ba71bd9
0077f2a
5c4db67
475e1e9
271e2f8
bdb7107
32b7599
df7ac7d
0b89f95
dc84933
7a66891
287bdaf
0d71d92
5c1999b
6b06a8d
6552d8d
d9742ad
8c3adb4
b298a36
4b21d82
e827153
57d9199
d6d8711
66d7856
5fc8490
0db6fbe
ba04f27
76e5a1a
f5e6c7e
791eb49
316a525
90aefd5
35954de
aa0a674
e91cb76
5d4c2ad
d2aa514
834032c
4855e7b
c02cf9a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The change to this function is interesting.
Perhaps we could look to overload the function by conditionally compiling 1 of 2 functions of the same name behind feature flags as opposed to modifying the body of one function for readability.
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.
Well mostly this code was repeated two times in the file and we already discussed this condition
is imo more idiomatic in if A / else if B / else. It's quite complicate to have cargo check and cargo clippy accepting overloads, I tried to have 1/2 methods with both config accordingly and that never passed cargo check/clippy. Most of times the only accepted way is to simply have #[cfg] working like if statements. Otherwise most of the times a function will be missing on compilation if you have like --features-all
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.
The structure of the if else is fine, It's functionally equivalent and should work fine.
I do think that if you like the style suggestion I put on the sister PR against the node I would love to see the same style applied to the feature flag here. If you don't want to go with that style, consider the PR approved.
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.
Casper client do not seem to have cargo clippy so I can duplicate this method in the client without side effects apparently.
Please check that commit 4855e7b thanks