Skip to content

Modernize to allow for Webmon Implementation #12

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

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

Conversation

darshdinger
Copy link
Collaborator

@darshdinger darshdinger commented Jul 17, 2025

Short description of the changes:

Modernize plot_publisher codebase: Add type hints, improve security, enhance testing, and fix critical bugs

Long description of the changes:

This PR represents a comprehensive modernization of the plot_publisher package, bringing it up to current Python best practices and significantly improving its reliability, security, and maintainability.

Core Improvements

Type Safety & Code Quality:

  • Added comprehensive type hints throughout the codebase using Union, Optional, and proper type annotations
  • Improved function signatures with clear parameter and return types
  • Enhanced IDE support and code readability

Configuration System Modernization:

  • Converted configuration from simple class to modern dataclass with validation
  • Added Configuration.from_file() class method for easier configuration loading
  • Implemented robust error handling for invalid configuration parameters
  • Maintained backward compatibility with existing configuration files

Security Enhancements:

  • Made SSL verification configurable with secure defaults (verify_ssl=True)
  • Replaced blanket SSL warning suppression with targeted warning handling
  • Added support for configurable certificate-based authentication
  • Proper handling of SSL contexts and certificate validation

Error Handling & Validation

  • Added comprehensive input validation for all public functions
  • Implemented proper exception handling with meaningful error messages
  • Eliminated silent failures and None returns that could mask errors
  • Added parameter validation with clear error messages for invalid inputs

Logging & Debugging

  • Replaced global logging calls with proper logger usage (logger = logging.getLogger(__name__))
  • Added debug, info, and error logging throughout the codebase
  • Enhanced traceability for troubleshooting production issues
  • Improved error reporting with detailed context

Testing Improvements

  • Updated all tests to use real Configuration objects instead of mocked dictionaries
  • Added comprehensive input validation tests for edge cases
  • Improved test fixtures and mocking strategies
  • Enhanced test coverage of critical code paths
  • Fixed previously failing publish_plot integration tests

Critical Bug Fixes

  • Fixed duplicate function definition: Removed erroneous duplicate _is_plotly_html_content function that was breaking publish_plot execution
  • Restored proper control flow: Fixed publish_plot function structure to ensure HTTP requests are actually made
  • Enhanced content detection: Improved _is_plotly_html_content logic for more robust plotly HTML detection
  • Eliminated silent failures: Fixed cases where HTTP requests were never executed due to code structure issues

Documentation Updates

  • Updated README.md with new Configuration usage examples
  • Enhanced docstrings with proper type information and parameter descriptions
  • Added comprehensive examples for the new Configuration API
  • Documented security features and SSL configuration options

Check list for the pull request

  • I have read the [CONTRIBUTING]
  • I have read the [CODE_OF_CONDUCT]
  • I have added tests for my changes
  • I have updated the documentation accordingly

Check list for the reviewer

  • I have read the [CONTRIBUTING]
  • I have verified the proposed changes
  • best software practices
    • all internal functions have an underbar, as is python standard
    • clearly named variables (better to be verbose in variable names)
    • code comments explaining the intent of code blocks
  • All the tests are passing
  • The documentation is up to date
  • code comments added when explaining intent

Manual test for the reviewer

Prerequisites:

Ensure you have a Python environment with the required dependencies (requests, plotly, numpy).

Testing Steps:

1. Run the Complete Test Suite:

# Run all tests to verify functionality
python -m pytest [test_plot_publisher.py](http://_vscodecontentref_/0) -v

# Expected: All 13 tests should pass

2. Test New Configuration System:

python -c "
from plot_publisher._configuration import Configuration

# Test new dataclass configuration
config = Configuration(
   publish_url_template='http://test.com/\${instrument}/\${run_number}',
   publisher_username='test',
   publisher_password='test'
)
print('✅ Configuration creation successful')
print('SSL verification (default):', config.verify_ssl)  # Should be True
"

3. Test Version Injection Functionality:

python -c "
from plot_publisher._plot_publisher import inject_plotlyjs_version

# Test plotly version injection
test_html = '<div id=\"plot-123\" class=\"plotly-graph-div\">Plot content</div>'
result = inject_plotlyjs_version(test_html, '5.15.0')

print('✅ Version injection successful')
print('Contains version metadata:', 'plotlyjs-version=' in result)
print('Contains version number:', '5.15.0' in result)
"

4. Test Public API:

python -c "
from plot_publisher import publish_plot, plot1d
print('✅ Public API imports successful')

# Test type hints are working (should show in IDE)
import inspect
print('publish_plot signature:', inspect.signature(publish_plot))
"

@backmari
Copy link

@coderabbitai full review

@@ -66,7 +67,43 @@ def add_version_attribute(match):
return modified_html


def publish_plot(instrument, run_number, files, config=None):
def inject_plotlyjs_version(html_content: str, version: str) -> str:

Choose a reason for hiding this comment

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

Why do we need this new version inject_plotlyjs_version, where the version is passed in as a string? I would think that detecting the version from the environment would be the preferred way?

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2025

Codecov Report

Attention: Patch coverage is 77.50000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 62.18%. Comparing base (6dfaa5c) to head (8672939).

Files with missing lines Patch % Lines
src/plot_publisher/_plot_publisher.py 80.35% 11 Missing ⚠️
src/plot_publisher/_configuration.py 68.18% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #12      +/-   ##
==========================================
+ Coverage   53.33%   62.18%   +8.85%     
==========================================
  Files           3        3              
  Lines         165      201      +36     
==========================================
+ Hits           88      125      +37     
+ Misses         77       76       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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