Source Code cleanups and improvements based on feedback from https://news.ycombinator.com/user?id=necovek

The initial Cursor/claude-3.7-sonnet(thinking) prompt was:

I have received the following feedback regarding this codebase:

"The premise might possibly be true, but as an actually seasoned Python developer, I've taken a look at one file: @utils.py. All of it smells of a (lousy) junior software engineer: from configuring root logger at the top, module level (which relies on module import caching not to be reapplied), over not using a stdlib config file parser and building one themselves, to a raciness in load_json where it's checked for file existence with an if and then carrying on as if the file is certainly there..."

I therefore ask you to thoroughly improve the code quality of the implementation in @src   while staying in line with the requirements from @REQUIREMENTS.md, and while ensuring that the Quality Tools (see @makefile) won't fail. Also, make sure that the tests in folder @tests  don't break.

See file @pyproject.toml for the general project setup. There is already a virtualenv at @venv.

--

This was followed by another prompt:

Check the other files in @src and identify where improvements are needed. Then do the improvements while ensuring Code Quality with the available quality tools and tests.

--

This was followed by another prompt:

Now improve ALL files @step1_prepare.py @step2_download_previous_state.py @step3_retrieve_hourly_problem_numbers.py @step4_generate_trend_chart.py @step5_download_logstash_documents.py @step6_extract_fields.py @step7_normalize_messages.py @step8_compare_normalizations.py @step9_generate_email_bodies.py @step10_send_email_report.py @step11_store_new_state.py @step12_cleanup.py using the same process.

--

A partial screen recording of the session is available at https://www.youtube.com/watch?v=zUSm1_NFKpA

Author: manuelkiessling

src/platform_problem_monitoring_core/__init__.py

@@ -3,4 +3,9 @@
 A tool for monitoring platform problems using Elasticsearch logs.
 """
 
-__version__ = "0.1.0"
+from importlib.metadata import PackageNotFoundError, version
+
+try:
+    __version__ = version("platform_problem_monitoring_core")
+except PackageNotFoundError:
+    __version__ = "0.1.0"  # Default version if package is not installed

src/platform_problem_monitoring_core/step10_send_email_report.py

@@ -3,6 +3,7 @@
 
 import argparse
 import smtplib
+import ssl
 import sys
 from email.mime.multipart import MIMEMultipart
 from email.mime.text import MIMEText
@@ -74,6 +75,7 @@ def send_email_report(
     smtp_pass: str,
     sender: str,
     receiver: str,
+    use_tls: bool = True,
 ) -> None:
     """
     Send email report.
@@ -88,6 +90,12 @@ def send_email_report(
         smtp_pass: SMTP password
         sender: Sender email address
         receiver: Receiver email address
+        use_tls: Whether to use TLS encryption (default: True)
+
+    Raises:
+        FileNotFoundError: If either email body file doesn't exist
+        smtplib.SMTPException: If there's an error sending the email
+        OSError: If there's an error reading the email body files
     """
     logger.info("Sending email report")
     logger.info(f"HTML file: {html_file}")
@@ -98,13 +106,28 @@ def send_email_report(
     logger.info(f"SMTP user: {smtp_user}")
     logger.info(f"Sender: {sender}")
     logger.info(f"Receiver: {receiver}")
+    logger.info(f"Use TLS: {use_tls}")
+
+    # Check if files exist
+    html_path = Path(html_file)
+    text_path = Path(text_file)
+
+    if not html_path.exists():
+        error_msg = f"HTML email body file not found: {html_file}"
+        logger.error(error_msg)
+        raise FileNotFoundError(error_msg)
+
+    if not text_path.exists():
+        error_msg = f"Text email body file not found: {text_file}"
+        logger.error(error_msg)
+        raise FileNotFoundError(error_msg)
 
     try:
         # Read the email bodies
-        with Path(html_file).open("r") as f:
+        with html_path.open("r") as f:
             html_body = f.read()
 
-        with Path(text_file).open("r") as f:
+        with text_path.open("r") as f:
             text_body = f.read()
 
         # Wrap long lines to avoid SMTP line length limits (RFC 5322 says 998 characters max)
@@ -125,21 +148,37 @@ def send_email_report(
         logger.info(f"Connecting to SMTP server {smtp_host}:{smtp_port}")
 
         # Send the email
-        server = smtplib.SMTP(smtp_host, smtp_port)
-        server.starttls()
-        server.login(smtp_user, smtp_pass)
-        server.sendmail(sender, receiver, msg.as_string())
-        server.quit()
-
-        logger.info("Email report sent successfully")
-    except FileNotFoundError as e:
-        logger.error(f"Email body file not found: {str(e)}")
-        raise
-    except smtplib.SMTPException as e:
-        logger.error(f"SMTP error: {str(e)}")
-        raise
-    except Exception as e:
-        logger.error(f"Unexpected error sending email: {str(e)}")
+        server = None
+        try:
+            server = smtplib.SMTP(smtp_host, smtp_port, timeout=30)
+
+            # Optional: Enable debug output
+            # server.set_debuglevel(1)
+
+            # Use TLS if requested
+            if use_tls:
+                context = ssl.create_default_context()
+                server.starttls(context=context)
+
+            server.login(smtp_user, smtp_pass)
+            server.sendmail(sender, receiver, msg.as_string())
+            logger.info("Email report sent successfully")
+        except smtplib.SMTPException as e:
+            error_msg = f"SMTP error: {str(e)}"
+            logger.error(error_msg)
+            raise
+        finally:
+            if server is not None:
+                server.quit()
+                logger.debug("SMTP connection closed")
+
+    except (OSError, smtplib.SMTPException) as e:
+        if isinstance(e, FileNotFoundError):
+            logger.error(f"Email body file not found: {e}")
+        elif isinstance(e, smtplib.SMTPException):
+            logger.error(f"SMTP error: {e}")
+        else:
+            logger.error(f"Error sending email: {e}")
         raise
 
 
@@ -155,6 +194,7 @@ def main() -> None:
     parser.add_argument("--smtp-pass", required=True, help="SMTP password")
     parser.add_argument("--sender", required=True, help="Sender email address")
     parser.add_argument("--receiver", required=True, help="Receiver email address")
+    parser.add_argument("--no-tls", action="store_true", help="Disable TLS encryption")
 
     args = parser.parse_args()
 
@@ -169,6 +209,7 @@ def main() -> None:
             args.smtp_pass,
             args.sender,
             args.receiver,
+            not args.no_tls,
         )
         sys.exit(0)
     except Exception as e:

src/platform_problem_monitoring_core/step11_store_new_state.py

@@ -6,7 +6,7 @@
 from pathlib import Path
 
 import boto3
-from botocore.exceptions import ClientError
+from botocore.exceptions import ClientError, NoCredentialsError
 
 from platform_problem_monitoring_core.utils import logger
 
@@ -20,6 +20,11 @@ def store_new_state(s3_bucket: str, s3_folder: str, date_time_file: str, norm_re
         s3_folder: S3 folder name
         date_time_file: Path to the date and time file to upload
         norm_results_file: Path to the normalization results file to upload
+
+    Raises:
+        FileNotFoundError: If either input file doesn't exist
+        NoCredentialsError: If AWS credentials are not found
+        ClientError: If any AWS S3 operation fails
     """
     logger.info("Storing new state")
     logger.info(f"S3 bucket: {s3_bucket}")
@@ -28,37 +33,62 @@ def store_new_state(s3_bucket: str, s3_folder: str, date_time_file: str, norm_re
     logger.info(f"Normalization results file: {norm_results_file}")
 
     # Check if files exist
-    if not Path(date_time_file).exists():
+    date_time_path = Path(date_time_file)
+    norm_results_path = Path(norm_results_file)
+
+    if not date_time_path.exists():
         error_msg = f"Date time file not found: {date_time_file}"
         logger.error(error_msg)
         raise FileNotFoundError(error_msg)
 
-    if not Path(norm_results_file).exists():
+    if not norm_results_path.exists():
         error_msg = f"Normalization results file not found: {norm_results_file}"
         logger.error(error_msg)
         raise FileNotFoundError(error_msg)
 
-    # Create S3 client
-    s3_client = boto3.client("s3")
-
-    # Upload date time file
-    date_time_key = f"{s3_folder}/current_date_time.txt"
     try:
-        logger.info(f"Uploading date time file to s3://{s3_bucket}/{date_time_key}")
-        s3_client.upload_file(date_time_file, s3_bucket, date_time_key)
-        logger.info("Date time file uploaded successfully")
-    except ClientError as e:
-        logger.error(f"Failed to upload date time file: {e}")
+        # Create S3 client
+        s3_client = boto3.client("s3")
+
+        # Test connection to S3 by checking bucket existence
+        s3_client.head_bucket(Bucket=s3_bucket)
+        logger.info(f"Successfully connected to S3 bucket: {s3_bucket}")
+
+        # Upload date time file
+        date_time_key = f"{s3_folder}/current_date_time.txt"
+        try:
+            logger.info(f"Uploading date time file to s3://{s3_bucket}/{date_time_key}")
+            s3_client.upload_file(date_time_file, s3_bucket, date_time_key)
+            logger.info("Date time file uploaded successfully")
+        except ClientError as e:
+            error_code = e.response.get("Error", {}).get("Code", "Unknown")
+            error_msg = f"Failed to upload date time file: Error {error_code} - {str(e)}"
+            logger.error(error_msg)
+            raise ClientError(e.response, e.operation_name) from e
+
+        # Upload normalization results file
+        norm_results_key = f"{s3_folder}/norm_results.json"
+        try:
+            logger.info(f"Uploading normalization results to s3://{s3_bucket}/{norm_results_key}")
+            s3_client.upload_file(norm_results_file, s3_bucket, norm_results_key)
+            logger.info("Normalization results uploaded successfully")
+        except ClientError as e:
+            error_code = e.response.get("Error", {}).get("Code", "Unknown")
+            error_msg = f"Failed to upload normalization results: Error {error_code} - {str(e)}"
+            logger.error(error_msg)
+            raise ClientError(e.response, e.operation_name) from e
+
+    except NoCredentialsError as e:
+        logger.error(f"AWS credentials not found: {e}")
         raise
-
-    # Upload normalization results file
-    norm_results_key = f"{s3_folder}/norm_results.json"
-    try:
-        logger.info(f"Uploading normalization results to s3://{s3_bucket}/{norm_results_key}")
-        s3_client.upload_file(norm_results_file, s3_bucket, norm_results_key)
-        logger.info("Normalization results uploaded successfully")
     except ClientError as e:
-        logger.error(f"Failed to upload normalization results: {e}")
+        if e.response.get("Error", {}).get("Code") == "NoSuchBucket":
+            logger.error(f"S3 bucket not found: {s3_bucket}")
+        else:
+            logger.error(f"AWS S3 error: {e}")
+        raise
+    except Exception as e:
+        logger.error(f"Unexpected error storing state: {e}")
         raise
 
     logger.info("New state stored successfully")
@@ -80,6 +110,7 @@ def main() -> None:
 
     try:
         store_new_state(args.s3_bucket, args.s3_folder, args.date_time_file, args.norm_results_file)
+        sys.exit(0)
     except Exception as e:
         logger.error(f"Error storing new state: {e}")
         sys.exit(1)

src/platform_problem_monitoring_core/step12_cleanup.py

@@ -2,51 +2,103 @@
 """Clean up work environment."""
 
 import argparse
+import logging
+import os
 import shutil
 import sys
 from pathlib import Path
+from typing import List
 
 from platform_problem_monitoring_core.utils import logger
 
 
-def cleanup_environment(work_dir: str) -> None:
+def _verify_safe_path(work_dir: Path) -> None:
     """
-    Clean up the work environment by removing the temporary work directory.
+    Verify that the path is safe to remove.
 
     Args:
-        work_dir: Path to the temporary work folder to remove
-    """
-    logger.info("Cleaning up work environment")
-    logger.info(f"Removing temporary work directory: {work_dir}")
-
-    # Convert to Path object
-    work_path = Path(work_dir)
+        work_dir: Path to verify
 
+    Raises:
+        ValueError: If the path is not a directory, doesn't exist, or doesn't look like a temporary work directory
+    """
     # Check if the directory exists
-    if not work_path.exists():
-        logger.warning(f"Work directory does not exist: {work_dir}")
-        return
+    if not work_dir.exists():
+        error_msg = f"Directory does not exist: {work_dir}"
+        raise ValueError(error_msg)
 
     # Check if the path is a directory
-    if not work_path.is_dir():
+    if not work_dir.is_dir():
         error_msg = f"Path is not a directory: {work_dir}"
-        logger.error(error_msg)
         raise ValueError(error_msg)
 
     # Verify that the directory looks like a temporary work directory
     # This is a safety check to avoid accidentally deleting important directories
-    if not work_path.name.startswith("platform_problem_monitoring_"):
+    if not work_dir.name.startswith("platform_problem_monitoring_"):
         error_msg = f"Directory does not appear to be a temporary work directory: {work_dir}"
-        logger.error(error_msg)
         raise ValueError(error_msg)
 
+
+def _list_remaining_files(work_dir: Path) -> List[str]:
+    """
+    List files remaining in the directory.
+
+    Args:
+        work_dir: Path to the directory
+
+    Returns:
+        List of files found in the directory
+    """
+    files = []
     try:
+        for root, _dirs, filenames in os.walk(work_dir):
+            for filename in filenames:
+                file_path = Path(root) / filename
+                files.append(str(file_path.relative_to(work_dir)))
+        return files
+    except (OSError, ValueError) as e:
+        logger.warning(f"Error listing files in {work_dir}: {e}")
+        return []
+
+
+def cleanup_environment(work_dir: str) -> None:
+    """
+    Clean up the work environment by removing the temporary work directory.
+
+    Args:
+        work_dir: Path to the temporary work folder to remove
+
+    Raises:
+        ValueError: If the path is not suitable for removal
+        OSError: If there's an error removing the directory
+    """
+    logger.info("Cleaning up work environment")
+    logger.info(f"Removing temporary work directory: {work_dir}")
+
+    # Convert to Path object
+    work_path = Path(work_dir)
+
+    try:
+        # Check if the path is safe to remove
+        _verify_safe_path(work_path)
+
+        # Optional: List files before deletion (for debugging if needed)
+        if logger.isEnabledFor(logging.DEBUG):
+            files = _list_remaining_files(work_path)
+            if files:
+                logger.debug(f"Files to be removed: {', '.join(files)}")
+
         # Remove the directory and all its contents
         shutil.rmtree(work_dir)
         logger.info(f"Successfully removed directory: {work_dir}")
-    except Exception as e:
-        logger.error(f"Error removing directory {work_dir}: {str(e)}")
-        raise
+
+    except ValueError as e:
+        # Non-fatal errors (directory doesn't exist or isn't a temp directory)
+        logger.warning(f"Skipping cleanup: {str(e)}")
+    except OSError as e:
+        error_msg = f"Error removing directory {work_dir}: {str(e)}"
+        logger.error(error_msg)
+        raise OSError(error_msg) from e
 
     logger.info("Cleanup complete")