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.
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
Update Scheduler to have a configurable block provider #7434 #7441
Update Scheduler to have a configurable block provider #7434 #7441
Changes from 19 commits
212712d
9314582
fbb1eb7
8ea5046
a2c3c97
88cf158
1ea6c26
b29cf54
0670883
ce52df4
16585b9
df2b959
279ed58
55b0acd
d20b051
39a6e6c
0feaccc
62a4592
2fdf53d
ae9e068
c96dd35
03fc0da
9041d6c
d7a8db1
b745976
b887564
2ba273a
4c54432
39ded63
e09547a
17b7aec
441f5d3
21683a4
4069c9e
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.
Docs? Devs using this pallet wont know what it is or how it should be used.
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.
addressed in commit 03fc0da
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.
Please check how we normally write these docs. This is a comment
//
instead of a rust doc.Also there should be one introduction sentence on what it does and then more elaborate paragraph following.
Just put yourself into the position of someone having never seen this before. What is it? How am i supposed to use it? And possible implications.
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.
addressed in commit -> d7a8db1
open question, I did try and put myself into the shoes for somebody new (was relatively easy, did not require any roleplaying haha) and I think this should be maybe a part of a larger readme? Not just for this pallet, but over multiple pallets explaining this?
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.
Runtime will implement the config trait they usually want documentation as precise as possible.
I agree crate documentation is also good, and in this PR case we can warn in both the crate doc and the config doc.
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.
added some docs now
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.
@seemantaggarwal adding documentation to public items in Rust is a must, please follow the advise given by other peers in this regard.
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.
Thanks @ggwpez and @kianenigma
I will keep this in mind going forward.
For the particular scenario, I should've reached out to the stakeholders, i.e. oliver, and Guillam, and acted more rapidly, to resolve the leftover and fill the gaps in my documentation.
I will keep this in ming going forward, and take more responsibility.
Thanks again :)
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.
why? using local block provider can be just ok
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 this case we want to always use the injected block number provider, so i think it makes sense.
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.
We should probably deprecate the migrations. They would not interact so well with changing the BN provider at the same time and it is a lot of legacy code
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.
Yeah, keeping the old migrations around is an aspiration that we gave up on a while ago
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.
I would make it everywhere like this to highlight it has not effect, plus a comment
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.
I see, thanks, I need to start it by 0 instead of 1, and just add a comment, that this should not make a difference to the working? I did not understand what is the comment I need to put