ultrako / LevelXML

A C# library that generates LevelXML for the Happy Wheels import box
MIT License
1 stars 0 forks source link

Reconsider where my library should throw exceptions #21

Closed ultrako closed 1 month ago

ultrako commented 2 months ago

Currently, I have an all or nothing approach, where whenever my library generates an Entity (or Target) that: Has no visual or physical effect on the level Is a black hole Crashes the user on level startup Causes a crash in the middle of the level (sound ID) The library will throw an exception, preventing the level from being generated at all.

The question is, should it? If I allow certain properties to hold values that cause this, it still would be possible externally to detect and warn the user about these issues. The fact that it's such an all-or-nothing approach means I've applied the aforementioned rules inconsistently: some levels need to use black holes, so I allow setting Density to NaN. Another example is that triggers that start disabled and don't have anything that can enable them, are allowed. But another, more fundamental issue with this approach is that I have to decide, "is there a use case for allowing this?", which maybe shouldn't be a question this library has to answer.

I have three alternative approaches to consider:

  1. Rather than throwing an exception, should I just log the extreme circumstance? Do I need to provide the users a way to configure what gets logged? An issue with this approach is that the way I've used this library, is to make applications that print out text to standard out - if I mix in logs with that, I'm not going to be able to copy and paste into the Happy Wheels import box. Should I print to stderr instead?
  2. I can do nothing when these situations are encountered, and rely on another component to warn users of problems with their levels. I have two options for this approach, though: a. Implement it as a separate library the end developer also has to import b. Implement it within the same library
ultrako commented 1 month ago

There are still certain situations on level import that should throw though; you could always place some random XML tag in the level that doesn't correspond to anything, and what the happy wheels import box will do is just delete it. I don't want to silent-delete things in my library though; if I do that it makes it impossible to debug what went wrong. There are also parameter values that would cause the game to silently delete the object - I don't want to allow those at all, and throw when that happens also.

ultrako commented 1 month ago

The library no longer throws on the aforementioned conditions as of commit https://github.com/ultrako/LevelXML/commit/935107adb077439026481f6d444a055627c808af. Not closing this yet because we still need to add back the ability to detect these conditions with a diagnostics component

ultrako commented 1 month ago

The character being set to NaN in the info tag is now neither in scope for the Level constructor to throw on, nor is detectable to the diagnostics component that just gets a list of entities. This is a problem because there could be a use case where someone imports a level via text, that level doesn't have a c attribute in the info tag, and then they run the diagnostics component on level.Entities and don't see anything, and can't diagnose why the level isn't starting. The solution is that I'll also expose a public method that takes a Level, runs that check on the level, and then runs the rest of the checks on level.Entities

ultrako commented 1 month ago

Added diagnostics component in commit https://github.com/ultrako/LevelXML/commit/b5b376d16e391442b1dd3aebff8ac61e18976e51.