-
Notifications
You must be signed in to change notification settings - Fork 5
Closed
Labels
documentationImprovements or additions to documentationImprovements or additions to documentationenhancementNew feature or requestNew feature or request
Description
Feedback for commit: 06bcdf0
Repository Structure:
- Why is everything inside lib? Is this idiomatic? Maybe makes sense for a src directory too.
- Lacking a Util module? Possibly useful
- Lacking a logging module. Definitely useful, should let users disable logging, enable logging and pipe logs to their own systems if they want to, is simple ash too. dprint from Board.luau:102 and SmartAssert from Board.luau:116 for example I would move here.
- General thing, promises + threads sounds like debugging hell.
- Sharding looks great
Board.luau:
- Please add a comment explaining what the file does at the top
- Explain what Shards are in the file ideally (Where you write ShardCounts, "Modify to match your MAU")
- Board.luau:92, is this global state?
- Board.luau:[101-181], this seems like a mix of Util and Shard utility functionality, should be in its own files imo
- Board.luau:306, does this wait for all the actions of threads to complete before killing them? Can this cause inconsistent states? See the thread spawned in the constructor, Board.luau:203.
Compression.luau:
- Again a file comment would be good.
- math.log is only defined for positive numbers, as long as this is OK with your Leaderboards it should be fine.
- Compression.luau:11, small optimisation, just pop the num~=0 check to outside the floor, and the function should be the same but just faster.
init.luau:
- init.luau:12, make this multiplication explicit for maintainability, rather than this wild number.
- init.luau:163, smartAssert returns! seriously just pop this to a nice util file
- init.luau:441, again check for data races when this is destroyed. Just make sure everything is consistent before tasks are cancelled.
MemoryShard.luau:
- MemoryShard.luau:13, this will lead to desynch between this and actual calendar months, this is obvs intentional, but might be worth supporting actual calendar months as well?
Retrospective comments:
- It is a bit hard to understand responsibility from reading just the code, this is where adding comments to the top of each file will be super useful.
- Reading docs, I am still a bit confused with the distinction between Leaderboards and Boards. Some more explanation here would be good.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
documentationImprovements or additions to documentationImprovements or additions to documentationenhancementNew feature or requestNew feature or request