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.

Sign up for GitHub

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

Add logging to save activity of app.py #25

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add logging to save activity of app.py #25

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3e44922
Added logging to save activity of app.py
JanviGundawar03 Oct 16, 2024
2d99c01
Added logging to save activity of app.py
JanviGundawar03 Oct 16, 2024
0d3267a
Revert "Added logging to save activity of app.py"
JanviGundawar03 Oct 16, 2024
b00740d
Added app.py
JanviGundawar03 Oct 17, 2024
bd42d48
implemented logging
JanviGundawar03 Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 40 additions & 10 deletions app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@
import tkinter as tk
import tkinter as ttk
from tkinter import Label, messagebox, Button

import logging
from PIL import Image, ImageTk, ImageSequence

from dotenv import load_dotenv

load_dotenv()

POPUP_DURATION = int(os.getenv("POPUP_DURATION", 60)) # Ensure these are integers
POPUP_INTERVAL = int(os.getenv("POPUP_INTERVAL", 60)) # Ensure these are integers
# Ensure these are integers
POPUP_DURATION = int(os.getenv("POPUP_DURATION", 60))
POPUP_INTERVAL = int(os.getenv("POPUP_INTERVAL", 60))


def resource_path(relative_path):
Expand All @@ -27,24 +28,36 @@ def resource_path(relative_path):
return os.path.join(base_path, relative_path)


# Configure logging
logging.basicConfig(
level=logging.DEBUG,
format='%(asctime)s - %(levelname)s - %(message)s',
handlers=[
logging.FileHandler('rsteye.log'),
logging.StreamHandler()
]
)
Comment on lines +31 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add missing trailing commas in the logging configuration.

Lines 37 and 38 in the handlers list are missing trailing commas, which can lead to syntax errors or unexpected behavior.

Apply this diff to fix the issue:

 logging.basicConfig(
     level=logging.DEBUG,
     format='%(asctime)s - %(levelname)s - %(message)s',
     handlers=[
         logging.FileHandler('rsteye.log'),
         logging.StreamHandler(),
+    ],
+)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Configure logging
logging.basicConfig(
level=logging.DEBUG,
format='%(asctime)s - %(levelname)s - %(message)s',
handlers=[
logging.FileHandler('rsteye.log'),
logging.StreamHandler()
]
)
# Configure logging
logging.basicConfig(
level=logging.DEBUG,
format='%(asctime)s - %(levelname)s - %(message)s',
handlers=[
logging.FileHandler('rsteye.log'),
logging.StreamHandler(),
],
)
🧰 Tools
🪛 Ruff

37-37: Trailing comma missing

Add trailing comma

(COM812)

38-38: Trailing comma missing

Add trailing comma

(COM812)


class RstEyeApp:
def __init__(self, image_path, interval=POPUP_INTERVAL * 60, fullscreen=False):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add missing type annotations to the __init__ method.

Adding type annotations improves code readability and helps with static analysis. The __init__ method is missing type annotations for self, image_path, interval, and fullscreen, as well as a return type annotation.

Consider updating the method signature as follows:

 def __init__(
     self,
     image_path: str,
     interval: int = POPUP_INTERVAL * 60,
     fullscreen: bool = False
 ) -> None:
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def __init__(self, image_path, interval=POPUP_INTERVAL * 60, fullscreen=False):
def __init__(
self,
image_path: str,
interval: int = POPUP_INTERVAL * 60,
fullscreen: bool = False
) -> None:
🧰 Tools
🪛 Ruff

42-42: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

42-42: Missing type annotation for self in method

(ANN101)

42-42: Missing type annotation for function argument image_path

(ANN001)

42-42: Missing type annotation for function argument interval

(ANN001)

42-42: Boolean default positional argument in function definition

(FBT002)

42-42: Missing type annotation for function argument fullscreen

(ANN001)

self.image_path = resource_path(image_path)
self.interval = interval
self.fullscreen = fullscreen
self.root = tk.Tk()
self.root.withdraw()
logging.info("Initialized RstEyeApp with image path: %s", self.image_path)

if not os.path.exists(self.image_path):
messagebox.showerror("Error", f"Image file '{self.image_path}' not found.")
logging.error(f"Image file '{self.image_path}' not found.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use logging parameterization instead of f-strings.

Using f-strings in logging statements can lead to unnecessary string interpolation when the log level is not enabled. Utilize logging's built-in string formatting to improve performance.

Apply this diff:

- logging.error(f"Image file '{self.image_path}' not found.")
+ logging.error("Image file '%s' not found.", self.image_path)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
logging.error(f"Image file '{self.image_path}' not found.")
logging.error("Image file '%s' not found.", self.image_path)
🧰 Tools
🪛 Ruff

52-52: Logging statement uses f-string

(G004)

self.root.destroy()
return

def show_image(self):
try:
popup = tk.Toplevel(self.root)
popup.title("Please Wait")

logging.info("Showing popup window...")
# Set the popup window size and position it in the center
popup_width = 350
popup_height = 200
Expand All @@ -54,6 +67,8 @@ def show_image(self):
y = (screen_height / 2) - (popup_height / 2)
popup.geometry(f"{popup_width}x{popup_height}+{int(x)}+{int(y)}")

logging.info(f"Popup window created at position ({x}, {y}) with size ({popup_width}x{popup_height}).")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use logging parameterization instead of f-strings.

Consistently use logging's string interpolation throughout your code for better performance and standardization.

Apply this diff:

- logging.info(f"Popup window created at position ({x}, {y}) with size ({popup_width}x{popup_height}).")
+ logging.info(
+     "Popup window created at position (%d, %d) with size (%dx%d).",
+     x,
+     y,
+     popup_width,
+     popup_height
+ )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
logging.info(f"Popup window created at position ({x}, {y}) with size ({popup_width}x{popup_height}).")
logging.info(
"Popup window created at position (%d, %d) with size (%dx%d).",
x,
y,
popup_width,
popup_height
)
🧰 Tools
🪛 Ruff

70-70: Logging statement uses f-string

(G004)


# Customize the popup window with a background image and better colors
background_image = ImageTk.PhotoImage(
Image.open(resource_path("rsteye.png")).resize(
Expand All @@ -74,7 +89,10 @@ def show_image(self):
# Add buttons to the popup window
button_frame = tk.Frame(popup)
button_frame.pack(pady=10)

except Exception as e:
logging.error(f"Failed to show image: {e}")
messagebox.showerror("Error", f"Failed to show image: {e}")

Comment on lines +92 to +95
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid catching broad exceptions and use logging.exception.

Catching all exceptions with except Exception as e can mask other issues and make debugging difficult. Instead, catch specific exceptions. Also, use logging.exception() to automatically include the traceback in your logs.

Apply this diff:

 except FileNotFoundError as e:
     logging.exception("Failed to show image.")
     messagebox.showerror("Error", f"Failed to show image: {e}")

Adjust the exception type to the most appropriate one for your context.

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 Ruff

92-92: Do not catch blind exception: Exception

(BLE001)

93-93: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

93-93: Logging statement uses f-string

(G004)

def load_image():
# Create the main window for the GIF
window = tk.Toplevel(self.root)
Expand Down Expand Up @@ -106,20 +124,26 @@ def update_frame(frame_index):
label.configure(image=frame)
frame_index = (frame_index + 1) % len(frames)
window.after(50, update_frame, frame_index)

window.after(0, update_frame, 0)
window.deiconify()

# Close the popup window after the GIF window is displayed
popup.destroy()

window.after(POPUP_DURATION * 1000, window.destroy)
logging.info(f"Image window will close after {POPUP_DURATION} seconds.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use logging parameterization instead of f-strings.

Again, replace f-strings with logging's parameterization for consistency and performance.

Apply this diff:

- logging.info(f"Image window will close after {POPUP_DURATION} seconds.")
+ logging.info("Image window will close after %d seconds.", POPUP_DURATION)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
logging.info(f"Image window will close after {POPUP_DURATION} seconds.")
logging.info("Image window will close after %d seconds.", POPUP_DURATION)
🧰 Tools
🪛 Ruff

135-135: Logging statement uses f-string

(G004)

except Exception as e:
logging.error(f"Error loading image sequence: {e}")
messagebox.showerror("Error", f"Failed to load image: {e}")
Comment on lines +136 to +138
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid catching broad exceptions and use logging.exception.

Catching broad exceptions can obscure unexpected errors. Using logging.exception includes the traceback, aiding in debugging.

Apply this diff:

- except Exception as e:
-     logging.error(f"Error loading image sequence: {e}")
-     messagebox.showerror("Error", f"Failed to load image: {e}")
+ except Exception as e:
+     logging.exception("Error loading image sequence.")
+     messagebox.showerror("Error", f"Failed to load image: {e}")

Consider catching specific exceptions relevant to image loading.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
except Exception as e:
logging.error(f"Error loading image sequence: {e}")
messagebox.showerror("Error", f"Failed to load image: {e}")
except Exception as e:
logging.exception("Error loading image sequence.")
messagebox.showerror("Error", f"Failed to load image: {e}")
🧰 Tools
🪛 Ruff

136-136: Do not catch blind exception: Exception

(BLE001)

136-136: try-except block with duplicate exception Exception

(B025)

137-137: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

137-137: Logging statement uses f-string

(G004)


# Define button actions
def on_accept():
logging.info("User accepted to view the image.")
self.root.after(0, load_image)

def on_exit():
logging.info("User declined to view the image.")
Comment on lines +142 to +146
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add missing return type annotations for on_accept and on_exit functions.

Including return type annotations enhances code clarity and supports better static analysis.

Update the function definitions as follows:

 def on_accept() -> None:
     logging.info("User accepted to view the image.")
     self.root.after(0, load_image)

 def on_exit() -> None:
     logging.info("User declined to view the image.")
     popup.destroy()

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 Ruff

145-145: Missing return type annotation for private function on_exit

Add return type annotation: None

(ANN202)

popup.destroy()

# Create buttons without specifying foreground or background colors
Expand All @@ -143,24 +167,30 @@ def on_exit():
relief="flat",
)
exit_button.pack(side="right", padx=20)

popup.mainloop()

except Exception as e:
messagebox.showerror("Error", f"Failed to load image: {e}")

def start_popup(self):
while True:
logging.info(f"Waiting {self.interval} seconds before showing the next popup.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use logging parameterization instead of f-strings.

Maintain consistency in logging statements by using parameterization.

Apply this diff:

- logging.info(f"Waiting {self.interval} seconds before showing the next popup.")
+ logging.info("Waiting %d seconds before showing the next popup.", self.interval)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
logging.info(f"Waiting {self.interval} seconds before showing the next popup.")
logging.info("Waiting %d seconds before showing the next popup.", self.interval)
🧰 Tools
🪛 Ruff

177-177: Logging statement uses f-string

(G004)

time.sleep(self.interval)
self.root.after(0, self.show_image)

def start(self):
logging.info("Starting the RstEyeApp.")
self.root.after(0, self.show_image)
threading.Thread(target=self.start_popup, daemon=True).start()
self.root.mainloop()


if __name__ == "__main__":
image_path = "med.gif"
app = RstEyeApp(image_path, fullscreen=True)
app.start()
try:
image_path = "med.gif"
logging.info("RstEyeApp is starting.")
app = RstEyeApp(image_path, fullscreen=True)
app.start()
except Exception as e:
logging.error(f"Unhandled exception occurred: {e}")
messagebox.showerror("Fatal Error", f"An error occurred: {e}")
Comment on lines +194 to +196
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid catching broad exceptions and use logging.exception.

Catching all exceptions may hide unexpected errors. Using logging.exception() will provide a traceback for better debugging.

Apply this diff:

- except Exception as e:
-     logging.error(f"Unhandled exception occurred: {e}")
-     messagebox.showerror("Fatal Error", f"An error occurred: {e}")
+ except Exception as e:
+     logging.exception("Unhandled exception occurred.")
+     messagebox.showerror("Fatal Error", f"An error occurred: {e}")

Consider handling specific exceptions that could occur during startup.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
except Exception as e:
logging.error(f"Unhandled exception occurred: {e}")
messagebox.showerror("Fatal Error", f"An error occurred: {e}")
except Exception as e:
logging.exception("Unhandled exception occurred.")
messagebox.showerror("Fatal Error", f"An error occurred: {e}")
🧰 Tools
🪛 Ruff

194-194: Do not catch blind exception: Exception

(BLE001)

195-195: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

195-195: Logging statement uses f-string

(G004)