-
Notifications
You must be signed in to change notification settings - Fork 10
[feature] Display skipped entries counters in inventory status #280
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
Changes from all commits
f589c82
7a816fd
0ef175d
7d18cc2
0b789de
d9038f9
e62d768
6468750
3491157
e5c5fc0
84704c3
d08fc95
7da800a
173e736
580a273
d041a7e
9cbc0c4
e79ec78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. L.150 : Why do we keep both _skippedCount and SkippedEntriesCount? Is one of them really necessary in addition to the other?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I simplified this.
We still keep So there is now no duplicated private counter field in |
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like InventoryProcessDatanow stores InventoryBuilders instead of a direct Inventories list, so GetInventories() is used to derive the actual Inventory objects. Is that intentional? Also, should GetInventories() return an empty list instead ofnullto avoid using! here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is intentional.
I also applied your nullable-safety point:
|
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.
This method maps InventoryBuildersto aList, but it returns nullwhenInventoryBuildersisnull. Is that intentional, or should it return an empty list instead? Also, are we sure every InventoryBuilderand itsInventory property are non-null here?
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.
Good catch, thanks. I updated this to make the contract explicit and non-nullable:
InventoryProcessData.InventoryBuildersis now initialized to an empty list ([]) and is non-nullable.GetInventories()now returnsList<Inventory>(non-nullable) and returns an empty list when there are no builders.BaseInventoryRunnerandFullInventoryRunnerwere updated to remove null-forgiving operators.On the second point:
IInventoryBuilder.Inventoryis non-nullable in the interface andInventoryBuilderinitializes it in its constructor, so we rely on that invariant here.