BHoM_Engine: Enhance the logging functionality by including ability to suppress logging if desired

[FraserGreenroyd]

Issues addressed by this PR

Fixes #1840

Test files

Available here

Changelog

• Add ability to suppress recording events
• Add ability to unsuppress recording events (events are recorded by default, this is to allow switching back on after being switched off)
• Add ability to toggle throwing error events as exceptions or not

Additional comments

I have noted some ToDo around documentation and potential further discussion. One item of further discussion is on defaulting throwing exceptions. Currently this is being defaulted to off, however, @alelom raised a point during discussion that we could default to on with BHoM_UI handling turning this off when it initialises to keep the UX on the UI side clean. Happy to debate, however, scope of this PR is limited to adding the functionality so I would do a separate joint PR with here and BHoM_UI if we decide we want to do that after this PR.

[travispotterBH]

Generally naming could be better, but also, I question the approach, and believe that we should protect against accidentally affecting the logging of event/warnings/errors outside of the place of implementation of this code.

[FraserGreenroyd]

Generally naming could be better, but also, I question the approach, and believe that we should protect against accidentally affecting the logging of event/warnings/errors outside of the place of implementation of this code.

Thanks for the speedy feedback @travispotterBH , much appreciated 🥇

I've gone through and updated the language from switch off to suppress as I think this is easier to handle within the code base. For example, it changed the variable areWeSwitchedOff to suppressEvents - turning it from a question variable to a statement variable and more in keeping with easy code readability. Hopefully this will also please @alelom (as he expressed the areWeSwitchedOff variable name could be clearer - which I think it now is) 😄

For your suggestion in the larger comment, I would agree it could be nice, however, it is key to note that BHoM does not attempt to standardise exact processes, because these must be flexible and that philosophy should align to our code when used by developers as much as it aligns for our users. Our logging system has been working well for a while and this PR scope is not to refactor the entire aspect, but to enhance it to provide flexibility that we don't currently have, which hopefully this does (particularly as we grow out some of our tools in the ZeroCode Prototypes, and AI/ML prototypes @alelom is working with) 😄

I do agree that there is a risk of the logging system being turned off for longer than it ought to be, and some form of automatic turn on would be nice. However, as a developer, I get frustrated when people try to protect me from myself, as I believe @adecler also feels from previous conversations. I would rather have the power to turn it off for as long as I want and if I forget to turn it back on then that's on me. Having an additional using block also risks the codebase drifting from being accessible to any professional in the AEC industry, regardless of their level of computational proficiency, as it'll add a layer of complexity to potentially all methods as opposed to very defined systems.

The risk of the user turning off the logging system is inherent with how we handle our code, by virtue of reflecting all methods into all our UIs. We deliberately decided that BHoM functionality looks the same whether you use it from a programming script, a Grasshopper script, an Excel spreadsheet, or any other interface that can expose C#, and this isn't a philosophy point that I think we should move away from at this stage. However, the risk is there and to mitigate against it, I have opted to not just exclude recording events entirely - they just get suppressed from being seen by users but they still get logged in the suppressedLog. I have also added a method to RetrieveSuppresedLog(), so that if anyone does accidently leave the suppression state on for too long, they can retrieve the events back to the main log 😄

I also expect we will be able to improve on this with iterations as we do with all things so I would limit the scope of this PR to introducing this functionality, and while it ought to be good functionality, we shouldn't necessarily aim to resolve all the issues we can foresee with it in this work - thus while I've marked the comment as resolved I'm very happy to continue debating it in an issue if you'd like 😄

[pawelbaran]

I am a big fan of the concept in general, as some of you may know 😉 I am with @travispotterBH when it comes to switching vs suppressing, definitely the latter sounds better to me. To further improve what we have now, I would consider start/stop suppressing instead of suppress/unsuppress (GitHub highlights 'unsuppress' in red as nonexistent in English 🙈). But this is just my opinion as a non-native speaker, so no prob with getting voted down

[FraserGreenroyd]

@BHoMBot check compliance

[bhombot-ci]

@FraserGreenroyd to confirm, the following actions are now queued:

• check code-compliance
• check documentation-compliance
• check project-compliance
• check branch-compliance
• check dataset-compliance
• check copyright-compliance

[bhombot-ci]

The check branch-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

[alelom]

Code changes make sense and work well (tried script).

Happy to approve also following our conversation @FraserGreenroyd .

I'm adding a few more related thoughts below, not directly connected to this PR -- can be subject for separate conversations. The tl;dr version is that I think that this change is useful, and that we can bring this further by saving the original Exception in the Event object (rather than just its StackTrace).

Keeping the original exception would allow us to re-raise it when needed, in certain contexts (namely, in any non-UI context).

Since at the moment exceptions are swallowed by the event then, when you call BHoM methods from a non-UI context, this is limiting, because you don't directly see if an error occurred during a method call. Think about when writing NUnit tests, or when calling from terminal or another non-UI runtime like RhinoCompute.

We can already optionally pass the Exception to RecordError, however if and when the Exception is re-raised, we lose the original type of Exception, while only the StackTrace is retained. Having the original Exception re-thrown would be particularly useful to write exception-targeting tests, which is particularly useful for TDD.

However, this would imply that need to we make sure that we always pass the original Exception to the RecordError method, in particular when calling RecordError from a catch clause.

[FraserGreenroyd]

When I rerun compliance checks, I will be dispensating code compliance because of this issue.

@BHoMBot check compliance

[bhombot-ci]

@FraserGreenroyd to confirm, the following actions are now queued:

• check code-compliance
• check documentation-compliance
• check project-compliance
• check branch-compliance
• check dataset-compliance
• check copyright-compliance

[FraserGreenroyd]

@BHoMBot check core
@BHoMBot check serialisation
@BHoMBot check null-handling

[bhombot-ci]

@FraserGreenroyd to confirm, the following actions are now queued:

• check core
• check serialisation
• check null-handling

[FraserGreenroyd]

@BHoMBot check versioning
@BHoMBot check installer

[bhombot-ci]

@FraserGreenroyd to confirm, the following actions are now queued:

• check versioning
• check installer

[Tom-Kingstone]

After testing, my changes were addressed correctly, and works as expected now. Happy to approve

[FraserGreenroyd]

@BHoMBot this is a DevOps instruction. I am requesting neutral checks on: unit-tests

[bhombot-ci]

@FraserGreenroyd I have provided neutral checks to the checks requested. These checks will need to be run properly to obtain full results.

[bhombot-ci]

FAO: @FraserGreenroyd
@FraserGreenroyd is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is code-compliance.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 21651490134

[FraserGreenroyd]

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 21651490134

[bhombot-ci]

@FraserGreenroyd I have now provided a passing check on reference 21651490134 as requested.

[albinber]

Code look good and after testing everything work as intended. Happy to approve

[FraserGreenroyd]

@BHoMBot check ready-to-merge

[bhombot-ci]

@FraserGreenroyd to confirm, the following actions are now queued:

• check ready-to-merge

Changes have been addressed

[FraserGreenroyd]

@BHoMBot check ready-to-merge

[bhombot-ci]

@FraserGreenroyd to confirm, the following actions are now queued:

• check ready-to-merge