feat: split up window post API into separate calls #1278
Conversation
cryptonemo
requested review from
dignifiedquire and
porcuquine
as code owners
There was a problem hiding this comment.
I think this is good. The general form and structure is much better than before and basically as I would expect. There are some rough edges in terms of shape/names and also where some assumptions feel too baked in. Getting this right is tricky, and as you suggest, we may need to make another pass to improve things even after moving forward to release this feature. I think one round of effort to incorporate at least the low-hanging fruit in my comments may be worthwhile. I'd also like to take one more look at it, perhaps after just a little more cleanup, in light of having had time to absorb it. As I said, I think everything looks right, and passing tests is good. However, there are ways a refactor like this could lead to hard-to-detect changes of meaning, so we should be sure before merging. Just a little more scrubbing (on your part) and study/thought (on mine) should get us there.
|
I don't have a enough knowledge to do a review about the correctness. I saw that there are quite some iterators which are not parallel ones. I don't know if that matter and I also know that many of them are likely hard to fix. Parallel iterators are hard to do with the current code, there are just too many side effects in the loops. In order to make it easier, the coding style would need to change to a more functional, side-effect free style. I guess making everything parallelizable is probably out of scope for this PR, I just wanted to mention it to keep it in mind for future refactors. |
cryptonemo
force-pushed
the
split-window-post-new
branch
from
50a2720
to
4139697
Compare
| assert_eq!(post_config.typ, PoStType::Winning); | ||
| }, | ||
| PoStType::Winning => { | ||
| assert_eq!(post_config.typ, PoStType::Winning); |
There was a problem hiding this comment.
This assertion is quite redundant here. The previous line literally guarantees this will never fail, so I think it can be removed. I would instead add a comment justifying the decision. Since I don't think the number of partitions is structurally guaranteed to be one, I would probably add a comment justifying this decision. That comment should make clear what change to the code base (even if it can't be conveniently checked here) would necessitate a change here.
There was a problem hiding this comment.
Agreed, that looks like a hold-over from the previous if/else branch change. Rather than update and push, I'm opting to merge this and update in a future PR.
An alternate PR/proposal to #1269. Winning PoSt is not working at the moment.