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

add autoformatting, linting, github actions #1

Merged
merged 9 commits into from
May 18, 2024
Merged

Conversation

brycedrennan
Copy link
Contributor

@brycedrennan brycedrennan commented May 18, 2024

PR Type

enhancement, tests


Description

  • Added and updated various Python files to enhance functionality and improve code organization.
  • Introduced new exception handling classes for better error management.
  • Updated and expanded test cases to cover new functionalities.
  • Configured GitHub Actions for automated linting and testing.
  • Updated Makefile with new commands for development convenience.
  • Updated and added new dependencies for development.

Changes walkthrough 📝

Relevant files
Enhancement
6 files
__init__.py
Add explanatory comment to __init__.py                                     

graphpyshop/init.py

  • Added a comment line to clarify the purpose of the file.
+1/-1     
__main__.py
Reorganize and clean up imports and formatting in __main__.py

graphpyshop/main.py

  • Reordered imports for better organization.
  • Removed unnecessary whitespace.
  • Minor formatting adjustments.
  • +7/-8     
    shopify_async_base_client.py
    Refactor and enhance error handling in shopify_async_base_client.py

    graphpyshop/extensions/shopify_async_base_client.py

  • Added imports for asyncio, base64, hashlib, hmac, and json.
  • Reorganized imports and removed duplicate imports.
  • Introduced new exception classes for specific error handling.
  • Refactored the retry logic in handle_async_request to simplify the
    code.
  • Added detailed logging and error handling.
  • +99/-93 
    shopify_bulk_queries_plugin.py
    Standardize import order in shopify_bulk_queries_plugin.py

    graphpyshop/extensions/shopify_bulk_queries_plugin.py

    • Reordered imports for consistency.
    +3/-2     
    shopify_generate_queries.py
    Major enhancements to query generation in shopify_generate_queries.py

    graphpyshop/extensions/shopify_generate_queries.py

  • Extensive refactoring and enhancements to query generation logic.
  • Improved error handling and logging.
  • Added new methods for handling GraphQL schema and generating queries.
  • +588/-185
    Makefile
    Add new utility commands to Makefile for development ease

    Makefile

  • Added new make commands for initializing the environment, code
    generation, building, and linting.
  • +60/-0   
    Tests
    1 files
    test_query_graphql_specific.py
    Enhance and expand tests for GraphQL query generation       

    tests/generate_queries/test_query_graphql_specific.py

  • Added import for subprocess and tempfile.
  • Added pytest fixtures for testing.
  • Enhanced existing tests with more detailed checks.
  • +47/-10 
    Configuration changes
    3 files
    pylama_matcher.json
    Configure GitHub actions for Python linting                           

    .github/pylama_matcher.json

  • Added a new JSON configuration for GitHub actions to handle Python
    linting errors.
  • +18/-0   
    checks.yml
    Introduce GitHub Actions workflow for automated Python checks

    .github/workflows/checks.yml

  • Added a new GitHub Actions workflow for Python checks including
    linting and testing.
  • +157/-0 
    ruff.toml
    Configure Ruff linter settings in ruff.toml                           

    tests/ruff.toml

    • Added a new TOML configuration file for the Ruff linter.
    +15/-0   
    Dependencies
    2 files
    requirements-dev.in
    Update development dependencies in requirements-dev.in     

    tests/requirements-dev.in

    • Added new entries for build and setuptools dependencies.
    +6/-0     
    requirements-dev.txt
    Refresh development dependencies in requirements-dev.txt 

    tests/requirements-dev.txt

  • Updated the development dependencies list to reflect new and updated
    packages.
  • +93/-0   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added enhancement New feature or request tests labels May 18, 2024
    Copy link

    PR Description updated to latest commit (2c8a07a)

    Copy link

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    4, because the PR includes a wide range of changes across multiple files including Python scripts, GitHub Actions configurations, and Makefile. The changes involve code reorganization, new functionalities, and configuration for automated processes which require a thorough review to ensure correctness and adherence to best practices.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: In the AsyncLimiterTransport class within shopify_async_base_client.py, the method handle_async_request raises AsyncLimiterError with a formatted message. However, the formatting uses {self.MAX_RETRIES - retries + 1} which might not behave as expected due to order of operations in subtraction and addition. It should be reviewed to ensure the calculation reflects the intended retry count accurately.

    Code Quality: In shopify_generate_queries.py, the method generate_queries uses a complex and deeply nested logic which could be simplified or broken down into smaller methods to improve readability and maintainability.

    🔒 Security concerns

    No

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for missing schema files to prevent runtime failures

    To improve the robustness of the schema file handling, add an else clause to handle the
    case where the schema file does not exist and cannot be created, possibly raising an
    exception or logging an error.

    graphpyshop/main.py [33-37]

     if os.path.exists(schema_path):
         logging.info(f"Schema found at {schema_path}")
         settings.schema_path = schema_path
     else:
    -    logging.info("Schema not found, will write schema to file")
    +    logging.error("Schema not found and cannot be written to file. Please check permissions.")
    +    raise FileNotFoundError("Schema file is missing and cannot be created.")
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by adding robust error handling for a missing schema file scenario. It's crucial for preventing failures in production environments.

    9
    Increase the maximum string length for lint error messages to reduce false positives

    The max-string-length in the [lint.flake8-errmsg] section seems restrictive and may lead
    to unnecessary linting errors for longer error messages. Consider increasing the limit to
    accommodate more descriptive messages.

    tests/ruff.toml [15]

    -max-string-length = 50
    +max-string-length = 120
     
    Suggestion importance[1-10]: 6

    Why: Increasing the max-string-length for lint error messages is a reasonable suggestion to avoid cutting off useful information in error messages, though not critical.

    6
    Use a more flexible version specification for better compatibility

    The version pinning for certifi is set to a specific future version, which might lead to
    compatibility issues if the expected version is not available. Consider using a more
    flexible version specification.

    tests/requirements-dev.txt [15]

    -certifi==2024.2.2
    +certifi>=2024.2.2
     
    Suggestion importance[1-10]: 5

    Why: Suggesting a more flexible version specification for certifi is a good practice to avoid potential future compatibility issues, but the impact is moderate since it depends on the availability of the specific future version.

    5
    Error handling
    Add error handling for JSON parsing to improve robustness

    Implement error handling for JSON parsing within the streaming block to catch and handle
    potential JSON decoding errors gracefully.

    graphpyshop/extensions/shopify_async_base_client.py [372]

    -parsed_line = json.loads(line)
    +try:
    +    parsed_line = json.loads(line)
    +except json.JSONDecodeError as e:
    +    logging.error(f"Failed to parse line: {line}, error: {e}")
    +    continue
     
    Suggestion importance[1-10]: 8

    Why: Adding error handling for JSON parsing is crucial to prevent crashes from malformed data and improve the robustness of the application.

    8
    Best practice
    Use tempfile.TemporaryDirectory to manage temporary files more safely and cleanly

    Instead of using tempfile.NamedTemporaryFile with delete=False, consider using
    tempfile.TemporaryDirectory as a context manager to ensure that all temporary files are
    cleaned up after the test runs. This approach avoids potential issues with undeleted files
    on the filesystem.

    tests/generate_queries/test_query_graphql_specific.py [33-37]

    -with tempfile.NamedTemporaryFile(
    -    delete=False, suffix=".graphql"
    -) as actual_file:
    -    actual_file.write(actual_query.encode())
    -    actual_file_path = actual_file.name
    +with tempfile.TemporaryDirectory() as temp_dir:
    +    actual_file_path = os.path.join(temp_dir, "actual.graphql")
    +    with open(actual_file_path, 'w') as actual_file:
    +        actual_file.write(actual_query.encode())
     
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a best practice for handling temporary files in Python, ensuring cleanup and avoiding potential filesystem clutter. This is a significant improvement in the test environment setup.

    8
    Use version ranges for dependencies to enhance flexibility and prevent conflicts

    The dependencies listed are directly tied to specific versions, which can lead to rigid
    upgrade paths and potential conflicts. Consider using version ranges to allow more
    flexibility in dependency resolution.

    tests/requirements-dev.txt [3-11]

    -annotated-types==0.6.0
    -anyio==4.3.0
    -ariadne-codegen==0.13.0
    -autoflake==2.3.1
    -black==24.4.2
    +annotated-types>=0.6.0,<0.7
    +anyio>=4.3.0,<5
    +ariadne-codegen>=0.13.0,<0.14
    +autoflake>=2.3.1,<3
    +black>=24.4.2,<25
     
    Suggestion importance[1-10]: 8

    Why: Using version ranges instead of pinning specific versions is a best practice in dependency management. It allows for more flexibility and reduces the risk of conflicts, which is crucial for maintaining a healthy project dependency tree.

    8
    Performance
    Use Python's built-in difflib for generating diffs to avoid external process overhead

    Replace the subprocess call with a direct Python library for diffing, such as difflib,
    which can provide a more integrated and potentially faster solution than spawning a new
    process.

    tests/generate_queries/test_query_graphql_specific.py [45-53]

    -subprocess.run(
    -    [
    -        "cursor",
    -        "--diff",
    -        actual_file_path,
    -        expected_file_path,
    -        "--reuse-window",
    -    ]
    -)
    +import difflib
    +with open(actual_file_path) as f1, open(expected_file_path) as f2:
    +    diff = difflib.unified_diff(
    +        f1.readlines(), f2.readlines(), fromfile='actual', tofile='expected'
    +    )
    +    for line in diff:
    +        print(line)
     
    Suggestion importance[1-10]: 7

    Why: This suggestion offers a Python-native approach to handling diffs, which can be more efficient and integrated than using an external process. It's a good performance improvement, though not critical.

    7
    Conditionally log debug messages based on the logging level to enhance performance

    Optimize the logging by reducing the verbosity of debug logs and ensuring that they are
    only logged when necessary, to avoid cluttering the log files and potentially impacting
    performance.

    graphpyshop/extensions/shopify_async_base_client.py [377-389]

    -logging.debug(f"Found child with parent ID: {parent_id}")
    -logging.debug(f"Parent found for ID {parent_id}")
    -logging.debug(f"Type match found for {parsed_line['__typename']} under attribute {parent_field}")
    -logging.debug(f"Child appended to parent under {parent_field}.")
    +if logging.getLogger().isEnabledFor(logging.DEBUG):
    +    logging.debug(f"Found child with parent ID: {parent_id}")
    +    logging.debug(f"Parent found for ID {parent_id}")
    +    logging.debug(f"Type match found for {parsed_line['__typename']} under attribute {parent_field}")
    +    logging.debug(f"Child appended to parent under {parent_field}.")
     
    Suggestion importance[1-10]: 6

    Why: This suggestion is valid as it optimizes performance by reducing unnecessary logging, especially in production environments where debug logging might be disabled.

    6
    Enhancement
    Refine the file exclusion list for linting to improve performance and relevance

    Consider using a more specific exclusion list in extend-exclude to prevent linting of
    files that should not be checked, such as migrations or external libraries. An empty list
    means all files are included, which might not be optimal.

    tests/ruff.toml [1]

    -extend-exclude = []
    +extend-exclude = ['migrations/', 'external_libs/']
     
    Suggestion importance[1-10]: 7

    Why: The suggestion to specify files to exclude from linting is valid and can improve linting efficiency by avoiding unnecessary checks on files that should not be linted.

    7
    Maintainability
    Ensure the code clarity by either enabling or removing the commented-out configuration line

    The commented-out line # settings.write_schema_to_file = True should either be removed or
    uncommented if it's necessary for functionality. Leaving it commented might confuse future
    maintainers about its purpose.

    graphpyshop/main.py [38]

    -# settings.write_schema_to_file = True
    +settings.write_schema_to_file = True
     
    Suggestion importance[1-10]: 5

    Why: The suggestion correctly points out a maintainability issue with commented code that could confuse maintainers. It's a valid point for code clarity, though the impact on functionality is minimal.

    5

    @brycedrennan brycedrennan merged commit 0b66ea1 into main May 18, 2024
    18 of 19 checks passed
    @brycedrennan brycedrennan deleted the bd/build-fixes branch May 18, 2024 23:30
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant