Skip to content

Conversation

HeroponRikiBestest
Copy link
Contributor

PR relies on SabreTools/SabreTools.Serialization#23

Like that PR currently is as I'm writing this message, this is a draft for now. There isn't much point looking it over until I un-draft it, although this PR is admittedly far simpler than the Serialization one, so there won't be as much for me to fix/change.

I will un-draft it at the same time as the serialization one, as it probably helps to understand why I want certain things in the serialization one.

@HeroponRikiBestest
Copy link
Contributor Author

PR is ready for review.

Responses to pointers you gave me in discord:

Line wrapping and newlines being inconsistent

I apologize for however many I've missed, but I went through the things you told me to look for there and caught the ones I noticed

Out-of-place / copy-paste comments

All non-todo comments seem ok to me after a pass

Copy-paste naming of methods

I think I've caught all these

"Personal" comments

Every remaining TODO (save for maybe 1 or 2) is a personal comment, because it's me wanting confirmation or otherwise from you. Assume every TODO I've left is asking you something, unless it really seems like it isn't. Otherwise, there shouldn't be any other personal comments

Naming or something like that

There's probably some naming issues, but I wouldn't be able to determine them on my own.

Make sure access levels make sense, e.g. does something need to be private or public?

My current access levels definitely don't make sense. I hate to do this, but this is probably singlehandedly my greatest weakness in terms of programming; every time I make something private or static it winds up completely destroying my program, and every fix feels awful. I'll need your advice on what publics can be made private and what non-statics can be made static upon your review

Check to see if IO.Extensions has more things you can use since I threw a lot of helpers in there

I think I'm using everything I can that I should, but let me know if you see any areas where some more extensions would be useful.

Ensure that the deserializers have as little "custom" stuff as possible in them. If they're in the deserializer, they probably need to be used in the wrapper too and so they should live somewhere else

I think I've done this

Default values and types are fine more often than you think, so using var when the righthand side is obvious. Or defining an enum as just enum without the extra type extension is fine if you never have to care about the bit width

Just went through and did this.

This is kinda new, but mostly I'm trying to enforce it for myself more: when writing a deserializer, try to avoid putting too much logic in the main method. Generally it makes it easier to have the main method be the overall output and then each helper method be specific to a type. Gets weird when there's a single type with no nesting, but I have examples of that.

Just went and did that.

It needs a separate lock to ensure that multiple threads won't try to generate the data at the same time

Did that

You need to have a way of determining whether a cached value has either never been created or shouldn't be created due to something like the required section is missing

Did that

You can make a helper method in the Sections region to do the actual creation of the wrapper if you need to, otherwise keep it in the getter of the property

It's currently in the Sections region; let me know if I can move it to the extension properties section or not.

@HeroponRikiBestest HeroponRikiBestest marked this pull request as ready for review September 18, 2025 18:17
@mnadareski mnadareski merged commit f263781 into SabreTools:master Sep 20, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants