Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Use pathlib on platformdirs folders #1484

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

rockstorm101
Copy link
Collaborator

Here is some refactoring to clean up the code base a bit. Nothing in this PR is written in stone so please feel free to reject/criticize/suggest changes. I tried to make commits self-explanatory but here is a summary:

printrun/pronsole:

  • Use more modern pathlib.Path instead of os.path for platformdirs folders
  • Initialize app dirs right after variable creation to avoid checking for
    their existence all the time.
  • Read/write files using with statements instead of manually opening and
    closing them.
  • Remove data_dir variable which is not used.

printrun/pronterface:

  • History file and cache dir are created during pronsole initialization so no
    need to check for their existence.

printrun/utils:

  • Update read/write history methods to use a pathlib.Path file instead of a file
    name. Existence check is removed from these functions because such check
    belongs to caller functions instead.
  • Simplify lookup_file function and their respective callers
  • sharedfile is not used and therefore removed

custombtn.txt was deprecated before Printrun 2.x but the code was left. The
code didn't even work as expected so this potentially breaking change has a
very low chance of affecting any users.
Closes kliment#1236

The `configfile` function was outdated and only used for handling
custombtn.txt so it is safe to remove as well.
Closes kliment#1286
 * Use more modern pathlib.Path instead of os.path.
 * Initialize app dirs right after variable creation to avoid checking for
   their existence all the time.
 * Read/write files using `with` statements instead of manually opening and
   closing them.
 * Remove `data_dir` variable which is not used.
History file and cache dir are created during `pronsole` initialization so no
need to check for their existence.
Update read/write history methods to use a pathlib.Path file instead of a file
name. Existence check is removed from these functions because such check
belongs to caller functions instead.
`sharedfile` is not used and therefore removed
@rockstorm101 rockstorm101 changed the title Refactor os path refactor: Use pathlib on platformdirs folders Feb 23, 2025
@DivingDuck
Copy link
Collaborator

Hi, just did a quick test on Windows and got an error:
Pronterface#412

The with statements can only be grouped since Python 3.10. Fall back to nested
with statements for now.
@rockstorm101
Copy link
Collaborator Author

Hi, just did a quick test on Windows and got an error:

Many thanks for testing. I just pushed a commit hoping to fix that one. Let me know if that works now. Also fixed a regression that was picked up by the GitHub Actions.

@DivingDuck
Copy link
Collaborator

DivingDuck commented Feb 23, 2025

That one is working fine now, but found another one:

I got an logging error when Log to console is activated. I did a connect and disconnect. When I then try to close the Pronterface Window I'm not able to do so and got the second traceback w/o been able to close Pronterface. I need to kill the process manually.
Maybe also a path problem with dirs as I can't see any update in the log file.

Edit: I forgot to mention that the errors happen only if I run from the binary file - running from source is fine.

Terminal output:

Verbinde...
--- Logging error ---
Traceback (most recent call last):
Traceback (most recent call last):
Keine Verbindung zum Drucker.
--- Logging error ---
Traceback (most recent call last):
  File "logging\__init__.py", line 1153, in emit
  File "printrun\pronterface.py", line 113, in write
AttributeError: 'NoneType' object has no attribute 'write'

During handling of the above exception, another exception occurred:


Traceback (most recent call last):
  File "printrun\printcore.py", line 171, in logError
  File "printrun\pronsole.py", line 284, in logError
  File "logging\__init__.py", line 2162, in error
  File "logging\__init__.py", line 1548, in error
  File "logging\__init__.py", line 1664, in _log
  File "logging\__init__.py", line 1680, in handle
  File "logging\__init__.py", line 1736, in callHandlers
  File "logging\__init__.py", line 1026, in handle
  File "logging\__init__.py", line 1158, in emit
  File "logging\__init__.py", line 1074, in handleError
  File "printrun\pronterface.py", line 113, in write
AttributeError: 'NoneType' object has no attribute 'write'

--- Logging error ---
Traceback (most recent call last):
Traceback (most recent call last):

@rockstorm101
Copy link
Collaborator Author

I got an logging error when Log to console is activated. I did a connect and disconnect.

Can you confirm this error does not happen on the master branch as well? Because the code regarding logging is not affected by the PR I believe. I'm unable to reproduce the behavior.

@DivingDuck
Copy link
Collaborator

Hi,
I just check my files. It happen in the master branch too. Looking back, I'm not sure that I had test this exact combination. The error only happen when I switch from not logging to stdout to logging to stdout and then restart Pronterface and there only if I run from binaries. Running from source is fine and do not show the behavior.

I need to take a closer look what happen with introducing this change (#1473) and why it fails in such a strange way and more over why there is no entry with the error in the log file as well...

Anyway, with this observation your PR works fine and we need to address the logging problem as an new issue.

@neofelis2X
Copy link
Collaborator

Works also fine on macOS. 🚀

@DivingDuck
Copy link
Collaborator

@rockstorm101, I created a new issue for my finding regarding the logging issue #1485.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants