Closed FraserGreenroyd closed 6 months ago
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 😄
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
@BHoMBot check compliance
When I rerun compliance checks, I will be dispensating code compliance because of this issue.
@BHoMBot check compliance
@BHoMBot check core @BHoMBot check serialisation @BHoMBot check null-handling
@BHoMBot check versioning @BHoMBot check installer
@BHoMBot this is a DevOps instruction. I am requesting neutral checks on: unit-tests
@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 21651490134
@BHoMBot check ready-to-merge
@BHoMBot check ready-to-merge
Issues addressed by this PR
Fixes #1840
Test files
Available here
Changelog
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.