Improve logger class #112
Improve logger class #112
Conversation
|
Hey Blake! These changes look solid to me. I noticed that the new SonarCloud check is failing though, and I believe it's on a part of the code on main that was brought in by @dbgen1 in #96.
Seems to be a pretty straightforward fix to me. Not sure if we want to capture it in this PR or make a unique patch for it on main. |
Appreciate you taking the time to look things over Michael With the earlier commit, some pytests were failing due to me not using a mock for the byte array. With @nateinaction 's help, he guided me to steps I needed to take to address the pytest errors. Now the tests are passing, |
@Mikefly123 @nateinaction |
blakejameson
force-pushed
the
improve-logger-class
branch
from
63728ea to
f1e6266
Compare
|
@nateinaction after rebasing 10 minutes ago, I tried running the code again on the board just to verify things still worked. 
I got hit with the error above. Adding static_method above the safe_init function is how I resolved the Sonar errors a few days back. I tested the code on the FC board after adding static_method 2 days ago, but now I realize I obviously messed up by either dragging an older version of pysquared.py or being on the wrong branch. Anyways, dealing with the Sonar errors for this safe_init function will likely take some refactoring of the safe_init functions because using the 'self' or 'cls' keyword didn't fix things. I am curious to know if you'd be open to reviewing this PR (with Sonar errors) and allowing the changes to be merged in on potential approval and if we could dedicate another issue towards fixing the Sonar errors resulting from the safe_init functions. Let me know what your thoughts are |
|
@nateinaction Apologies if this whole sequence is a lot to read through, but I went ahead and put back a parameter in the safe_init function of 'error_severity'. During my PR initially, I removed this parameter because it isn't needed. The logger outputs the severity so this string is unnecessary. However, for whatever reason, having this parameter in the function allows for no Sonar errors, though it marks it as an issue. All that being said, I confirm I ran the code on an v4b FC board and the output is similar to main branch |
|
@blakejameson what if we just passed |
|
Hi there, if you want to remove the error severity, there is actually no need for an additional layer to the decorator. The decorator currently consist of:
Theoretically, all the problems can be solved by simply removing the outer layer and renaming |
|
@Mikefly123 I am unable to see the Sonar reports before the most recent, but when I followed the recommendation of putting 'self', it resulted in 6 errors for the functions that used the decorator. I commented about it higher up in this thread |
|
@dbgen1 Thank you for explaining Davit. |
|
@blakejameson Ready to go! |
|
@dbgen1 Thank you so much for the fix!! I just ran the code on a v4b board and things worked normal @nateinaction After Davit's help in fixing the Sonar errors with safe_init, I can now hopefully say for the last time that the code is now ready for review |
lib/pysquared/logger.py
Outdated
| @@ -3,15 +3,51 @@ | |||
| Logs can be output to standard output or saved to a file (functionality to be implemented). | |||
| """ | |||
|
|
|||
| # NVM register number | |||
There was a problem hiding this comment.
| # NVM register number |
lib/pysquared/logger.py
Outdated
| self.logToStandardOut: bool = True | ||
| self.error_count: Counter = Counter(index=ERRORCNT, datastore=self.datastore) |
There was a problem hiding this comment.
Ok, looking at this now. I think I might have given bad advice. Instantiating this class inside the logger constructor doesn't feel right and violates our desired state of having a dependency injection framework. I think the logger interface in main and repl might instead be better off looking like:
logger: Logger = Logger(
counter=Counter(index=ERRORCNT, datastore=self.datastore)
)What do you think about trying to implement it that way?
There was a problem hiding this comment.
I am down for that!
There was a problem hiding this comment.
I don't have a finger on the scale in terms of design here, so up to you guys how to implement this! I did want to comment just to say this was the first time I had heard of a dependency injection framework, so I had to educate myself about it real quick.
I can definitely see the merits and how this is building toward's Nate's vision of having much more modular code vs the doom stack that pysquared.py and its children have been. I think there's perhaps an interesting paper here to write about how flight software gets written when your team isn't traditionally trained in embedded software engineering. It feels like the approaches are going to end up being quite a bit different!
There was a problem hiding this comment.
@Mikefly123 Isn't that a cool idea? Maybe not so surprisingly that's exactly what FPrime does. In addition to many other cool things, it's is a dependency injection framework building all objects it will ever need on start.
| @@ -24,35 +60,39 @@ def _log(self, level: str, message: str, **kwargs) -> None: | |||
|
|
|||
| json_output = json.dumps(kwargs) | |||
|
|
|||
| if self.logToStandardOut: | |||
| if self.logToStandardOut and self.can_print_this_level(level_value): | |||
| print(json_output) | |||
There was a problem hiding this comment.
If we so fancied we could return the debug color stuff here haha. Maybe something for a future PR
There was a problem hiding this comment.
Yeah! We talked about using the color interface from loguru as a guide for how we might reimplement it in our new logger. 🚀
| @@ -16,20 +16,20 @@ | |||
| from lib.pysquared.config import Config | |||
| from lib.pysquared.logger import Logger | |||
|
|
|||
| logger = Logger() | |||
|
|
|||
| logger: Logger = Logger(microcontroller.nvm) | |||
| logger.info("Booting", software_version="2.0.0", published_date="November 19, 2024") | |||
There was a problem hiding this comment.
This statement should actually probably be updated in each new PR to reflect this week's latest software version and the date that the PR was made ready for review. I think it's handy so users have an idea of what software they're running.
For this PR the version would be for V2.0.0-alpha-25w05 Pre-Release.
There was a problem hiding this comment.
I think I can solve for versioning in the build step soon!
There was a problem hiding this comment.
Will be very excited to see that come in!
lib/pysquared/logger.py
Outdated
| class LogMode: | ||
| PRINT = 1 | ||
| FILE = 2 | ||
| BOTH = 3 |
There was a problem hiding this comment.
Since LogMode is not fully implemented do you want to remove references to it in this PR?
lib/pysquared/logger.py
Outdated
| self.log_to_standard_out: bool = True | ||
| self.error_counter: Counter = error_counter | ||
| self.log_level: int = log_level | ||
| self.log_mode: int = log_mode |
There was a problem hiding this comment.
Since we don't want users to be able to modify these class member vars after they are set by the constructor, let's mark them as private by prefixing them with _.
Also ref the comment above about implementing log mode in a later PR we could remove self.log_to_standard_out and self.log_mode for this PR.
There was a problem hiding this comment.
Thank you for this call out
lib/pysquared/logger.py
Outdated
| def get_error_count(self) -> int: | ||
| return self.error_counter.get() |
There was a problem hiding this comment.
It looks like this isn't used anywhere but if we make the class member vars private then we will need this getter.
…to add the log_level and log_mode
…d, AllFaces, Faces, and other places. As the debug mode functionality is now originating in the Logger class, there is no need for extra debug variables. All debug or rather logmode and log levels will stem from the Logger
…oard, but it will result in Sonar errors
Co-authored-by: Michael Pham <[email protected]>
…tter method for error count
blakejameson
force-pushed
the
improve-logger-class
branch
from
c829b1d to
78f02a6
Compare
Co-authored-by: Nate Gay <[email protected]>
Co-authored-by: Nate Gay <[email protected]>
|
I removed the error_print() calls that were previously in pysquared.py. Error_count became a property of the Logger object. With the move to the Logger, it still remained as an nvm Counter object.
I added 2 new fields to the config.json, log_level and log_mode; I removed the debug field
I removed the debug parameter from Face, AllFaces, and Field. Being that the debut property or rather now the 'log_level' state is managed by the logger, there is no need for those classes to have a debug property and check for 'if debug', as that functionality is provided by the Logger
I can confirm I ran the code on a v4b board and saw similar output to what is currently in main, the only difference being the improved Logger error calls with cleaner output than previous.