Add FileHandler class

[tekktrik]

I needed to implement this class for keeping logs on an SD card I was already needed for file storage, I thought it might be a good addition to the library as a LoggingHandler

[tekktrik]

Updated to address issues in pre-commit

[tekktrik]

Also wanted to mention that I tested this with a Feather M4 Express and the Adalogger FeatherWing and got it to work!

[tekktrik]

Updated to be a better subset of CPython's FileHandler. file now stays open until closed, and arguments match (changed 'overwrite' to 'mode')

[FoamyGuy]

Can you also add a minimal example in the examples/ directory that shows the usage of the file handler.

[tekktrik]

Added a short example, had issues when actually importing the SD card stuff so I just tried to make it clear in the filepath. Let me know if it needs improvement.

[dastels]

The example should be able to run. I.e. the SD should be set up to be used in the example.
Apart from that it looks reasonable.

I do like keeping the file open between messages for performance.

Personally, I think additional handlers should be in separate files to avoid bloat in the main logging file. The serial & null handlers are core enough to be there, but additional handlers shouldn't come along as extra/unused baggage.

Finally, I feel that adding the line terminator belongs in the format method (which builds the message) and not emit (which sends the fully formed message to its destination).

Here's what I did in a project:

from adafruit_logging import LoggingHandler

class FileHandler(LoggingHandler):

    def __init__(self, filename):
        """Create an instance.
        :param filename: the name of the file to which to write messages
        """
        self._filename = filename

    def format(self, level, msg):
        """Generate a string to log.
        :param level: The level at which to log
        :param msg: The core message
        """
        return super(FileHandler, self).format(level, msg) + '\r\n'

    def emit(self, level, msg):
        """Generate the message and write it to the UART.
        :param level: The level at which to log
        :param msg: The core message
        """
        with open(self._filename, 'a+') as f:
            f.write(self.format(level, msg))

[tekktrik]

Should I make a folder named adafruit_logging, and add the main code as it's init.py, with FileHandler in it's own file?

[dastels]

That I'm not sure. Typical use is
import adafruit_logging as logging
to maintain a semblance of CPython usage. That would bring in everything in the directory which negates making it separate to avoid baggage. That isn't a big deal in this case, but setting a precedent now is better than later.

My understanding of python packaging is quite basic, but if the adafruit_logging.py is kept as is, and a directory named adafruit_logging_extansions added for this sort of thing, would that be acceptance/good? It would allow:

import adafruit_logging as logging
from adafruit_logging_extensions import FileHandler

Right?

Thoughts?

[tekktrik]

I think that could work as well. I think it really depends on what structure makes most sense to import. If it makes sense to have to totally removed than I think your proposal makes the most sense, but if it should still be under the adafruit_logging "umbrella", but separately imported then I think my solution might be better suited:

import adafruit_logging as logging
from adafruit_logging.extensions import FileHandler

In this case I would just put FileHandler in extensions.py, and the main code in __init__.py, all in a folder named adafruit_logging.

[tekktrik]

Open to whatever makes sense! To me, it seems reasonable to think that everything would be contained in something called adafruit_logging, with additional options pertaining to it to be available directly from itself.

[dastels]

import adafruit_logging as logging
from adafruit_logging.extensions import FileHandler

I like the look of that, but in general I'm not sure about extensions.py being a generic catch-all for extensions. That said, for now it should be fine ... there's only one thing in it. If it eventually starts accreting unrelated stuff it can be revisited.

[tekktrik]

Updated per everything said here, let me know what you think.

[dastels]

Looks good

[gamblor21]

@tekktrik do you know how to resolve the merge conflict / need any help with that?

[tekktrik]

I'd love some help with that actually!

[gamblor21]

I'd love some help with that actually!

I tried to merge it on the command line and I got an error about permissions. Not quite sure how to resolve this.
@kattni do you know who can help with this merge?

[kattni]

@gamblor21 You should be able to! I thought anyway. There's usually a checkbox that says "Allow edits from maintainers" or some such, but I don't see that here (possibly because I have higher perms than that box applies to so I don't see it on someone else's PR). The CircuitPythonLibrarians have write access to the repo, so it must be an issue on the PR itself. I should be able to help though! Do you have the changes somewhere I can get to so I can try to commit to this PR?

[kattni]

@gamblor21 I tried following the command line instructions that GitHub provides, and it appears to want to push to main which doesn't seem right. I thought it would involve updating something and pushing to this PR. I didn't complete the list of instructions - I stopped at git push origin main.

Edit - I think I see what I was about to do wrong. The command line instructions start with you working with tekktrik's fork, I think, which I did not do. That would explain both the permissions issues and the reason it wants to push to main.

[kattni]

@gamblor21 Yeah, the command line instructions provides are intended for @tekktrik to run, and expects permission to push to tekktrik's repository. I am getting the same results as you. We either need to figure out pushing to the PR, getting permissions from tekktrik to access their fork, or walking tekktrik through the process so they can complete it themselves. Dealer's choice!

[gamblor21]

@tekktrik do you see beside the greyed out "Merge pull request" there is a link about command line instructions? Can you click on that (it will expand) try to follow those?

[tekktrik]

Maybe I'm looking in the wrong place, but what I'm seeing is a grayed out box to resolve conflicts, but that I don't have the write access to do so. Should I be looking somewhere else?

Photo attached.

[kattni]

@tekktrik You're not looking in the wrong place - apparently GitHub doesn't provide you with the command line instructions. I am attaching a screenshot here. Hopefully this helps.

[tekktrik]

Just followed the instructions, did that help?

[tekktrik]

Do I need to create a new PR, or should I merge my new branch into this one that already has a PR?

[tekktrik]

Okay, I merged my local main branch into the one here with the PR after the instructions, which is commit 9fd0bc8, does that fix it?

[gamblor21]

Okay, I merged my local main branch into the one here with the PR after the instructions, which is commit 9fd0bc8, does that fix it?

No it is still showing the error. This is beyond my (limited) github experience, maybe someone else knows more who can help with it more.

[tekktrik]

I'm also happy to give access to my repo if that makes life easier :)

[gamblor21]

I'm also happy to give access to my repo if that makes life easier :)

You can give me access to it if you want and I can try it that way again.

[tekktrik]

I should have given you access, but I haven't ever done that before, let me know if I didn't.

[gamblor21]

I should have given you access, but I haven't ever done that before, let me know if I didn't.

That worked. I think I see the issue. You will have to pull the latest changes from the adafruit/main branch as your fork/branch has conflicts with the main one. You can set the remote with a command like git remote add adafruit git@github.com:adafruit/Adafruit_CircuitPython_Logging.git and then run git fetch adafruit and then you will have to rebase your code to the main branch git rebase adafruit/main.

I also saw this option in the github UI that may help.

Another option I can think of if this rebase seems large. Update your fork to the latest from adafruit and then reapply your changes in a new branch.

[tekktrik]

Got it! Thank you so much @gamblor21 and @kattni!!

[gamblor21]

Got it! Thank you so much @gamblor21 and @kattni!!

Thanks for sticking with it!