Logging Refactoring

[ykonyshev]

Implemented a global logging configuration as well as child loggers for modules. Replaced some printcalls and traceback.print_exc with corresponding logger.* calls. Separated out the argument parsing from the tagstudio/tagstudio.py to tagstudio/parse_args.py. Also ran ruff check --fix and added the ruff-pre-commit pre-commit hook. Refactored the code accordignly to satisfy the linter.

[ykonyshev]

@Loran425 Thank you for the review. I will have a look at the comments later, most likely tomorrow.

[CyanVoxel]

Definitely want @CyanVoxel's input on the match/case in tag_studio.py and the possibility of removing the ui folder, and I think there's some room to discuss the log formatter and what information should be displayed as well as some questions on the ContextVars and splitting out the arg parser, though that split has been growing on me.

It would probably be good to scrap the UI folder at this point. I think the UI development is pretty solidified in just Python at this point, for better or for worse. And I commented above but just in case it gets missed, I'm all for adding in match/case statements!

Thanks so much for your review on this!

[ykonyshev]

@Loran425 Thank you for the review. I will have a look at the comments later, most likely tomorrow.

I have started looking into the commets. I will need some time to implement the changes.

[ykonyshev]

Definitely want @CyanVoxel's input on the match/case in tag_studio.py and the possibility of removing the ui folder, and I think there's some room to discuss the log formatter and what information should be displayed as well as some questions on the ContextVars and splitting out the arg parser, though that split has been growing on me.

It would probably be good to scrap the UI folder at this point. I think the UI development is pretty solidified in just Python at this point, for better or for worse. And I commented above but just in case it gets missed, I'm all for adding in match/case statements!

Thanks so much for your review on this!

Am I understanding corretly that I can feel free to remove the UI folder as well as the holdover code relating to it? Or would you be willing to handle it in a separate PR?

[yedpodtrzitko]

It's generally a good practice to keep the changes relevant to the topic - if the title says it's about logging refactoring, then most likely it shouldnt contain a refactoring of argparser, or adding new ruff checks and changes related to it. Also if the ruff check will be considered compulsory, then it should be added into the CI pipeline as well, otherwise the next commit from someone who doesnt use pre-commit will introduce regression there.

[yedpodtrzitko]

on logging-relevant note. A nice improvement I could imagine would be using structlog, which is a drop-in replacement with nice formatting out of the box minus all the extra work like manually prefixing strings via {INFO} or {ERROR}, which is quite easy to forget to do.

It also allows to pass the variables as actual variables instead of mashing everything into an f-string. So for example this:

logger.info(f"Query:{query}, Frame: {i},  Length: {len(f)}")

can be written as this:

logger.info("Search Results", query=query, frame=i, length=len(f))

output looks like this:

[Loran425]

Definitely want CyanVoxel's input on the match/case in tag_studio.py and the possibility of removing the ui folder, and I think there's some room to discuss the log formatter and what information should be displayed as well as some questions on the ContextVars and splitting out the arg parser, though that split has been growing on me.

It would probably be good to scrap the UI folder at this point. I think the UI development is pretty solidified in just Python at this point, for better or for worse. And I commented above but just in case it gets missed, I'm all for adding in match/case statements! Thanks so much for your review on this!

Am I understanding corretly that I can feel free to remove the UI folder as well as the holdover code relating to it? Or would you be willing to handle it in a separate PR?

Since the addition of ruff check won't be happy with those lines not changing I would delete the commented lines and a separate PR can clean the files up if you want to make that as the files will be unreferenced and unused files at that point.

Unless this is getting broken up into 3 or so PR's 1 for arg parse, 1 for ruff, 1 for logging. in which case lots of changes would be needed.

---

It's generally a good practice to keep the changes relevant to the topic - if the title says it's about logging refactoring, then most likely it shouldnt contain a refactoring of argparser, or adding new ruff checks and changes related to it. Also if the ruff check will be considered compulsory, then it should be added into the CI pipeline as well, otherwise the next commit from someone who doesnt use pre-commit will introduce regression there.

@yedpodtrzitko that's just a tweak to the ruff work flow? or it needs a whole new workflow? Modified .github\workflows\ruff.yaml

name: Ruff
on: [ push, pull_request ]
jobs:
  ruff-format:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: chartboost/ruff-action@v1
        with:
          version: 0.4.2
          args: 'format --check'   
      - uses: chartboost/ruff-action@v1  #uses `check` arg by default
        with:
          version: 0.4.2

[yedpodtrzitko]

@yedpodtrzitko that's just a tweak to the ruff work flow? or it needs a whole new workflow? Modified .github\workflows\ruff.yaml

modifying the existing workflow like this should be sufficient

[ykonyshev]

Since the addition of ruff check won't be happy with those lines not changing I would delete the commented lines and a separate PR can clean the files up if you want to make that as the files will be unreferenced and unused files at that point. Unless this is getting broken up into 3 or so PR's 1 for arg parse, 1 for ruff, 1 for logging. in which case lots of changes would be needed.

I would be willing to make a separate PR to add the ruff check pre-commit hook as well as change the workflow. Should I also include the mypy pre-commit hook? As there is a CI workflow and isn't a pre-commit hook, so the only way to know whether mypy fails is to commit and then run the workflow.

[CyanVoxel]

Closing this as the codebase has changed significantly since this last had any activity. Thank you all for the valuable discussions here that will help inform future PRs.