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

Team 1 homework 3 - Gmail API Integration Implementation #9

Open
wants to merge 2 commits into
base: kyotov/conversations2
Choose a base branch
from

Conversation

Frankbz
Copy link

@Frankbz Frankbz commented Nov 3, 2024

This PR implements a Gmail class that provides a clean interface for interacting with the Gmail API, enabling email querying and management capabilities.

Copy link

@TechPertz TechPertz left a comment

Choose a reason for hiding this comment

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

Appreciate the effort put into this submission, but I noticed that this PR was opened just a few hours ago, rather than by the initial deadline of three weeks ago. This has led to some challenges:

The codebase includes multiple files with various functionalities and extensive changes, making it difficult to fully grasp.
There is an inadequate commit history, which makes it hard to follow the development process.
The code now includes advanced functionalities recently discussed in class, such as tool integrations and context management, making the review of the basic "communication with Gmail" more complex.

If there is any misunderstanding regarding the PR or commit history, please let me know.

src/gmail/API.py Outdated

Choose a reason for hiding this comment

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

Granularity:
Ensure that each function is committed separately for clarity and maintainability. Functions like query, get_message, get_labels, and search should ideally have their own commits.

Code Functionality:
Ensure all public functions handle edge cases gracefully and include error handling to manage API failures without crashing the program.
Verify that data structures returned by each function are correct and align with user expectations.

Readability & Structure:
The function and parameter names are clear, but enhancing docstrings with examples and more detailed descriptions will improve understanding.
Maintain a consistent logical flow within each function for better readability.

Standards Compatibility:
Confirm that the code passes linting tools and adheres to PEP 8 standards.
Ensure consistent type hints and uniform indentation and spacing.

API Exposure:
The all list limits the public API exposure to necessary functions, maintaining a clean module interface.
Ensure that only essential functions are exposed and internal logic is kept private for security and clarity.

Choose a reason for hiding this comment

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

Granularity:
Ensure separate commits for different functions and methods to maintain clarity and easy version tracking.

Code Functionality:
The authenticate() method should have checks for file integrity and scope validation to prevent errors during runtime.
Include error handling for API limits and potential response failures.

Readability & Structure:
The code structure is clean, but adding docstrings to methods like get_message and get_labels would improve clarity.
Address TODO comments promptly, such as the handling of attachmentId in the body() property.

Standards Compatibility:
Replace pprint with LOGGER.error() for structured logging and better error management.
Confirm adherence to PEP 8 standards and ensure the use of consistent naming conventions.

API Exposure:
Confirm that sensitive data and operations are protected and not inadvertently exposed.

Choose a reason for hiding this comment

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

Granularity:
Each class and significant method should be committed separately to ensure granular change tracking.

Code Functionality:
Ensure that the body() method correctly handles cases involving attachmentId.
Verify that methods like as_dict() and as_md() are robust enough to handle deeply nested parts without breaking.

Readability & Structure:
Add docstrings to properties and methods to improve code comprehension.
Ensure consistent naming and logical flow for readability.

Standards Compatibility:
Replace pprint with structured logging (LOGGER.error()) for consistent error handling.
Ensure type hints are consistent and meet PEP 8 standards.

API Exposure:
Confirm that only the necessary parts of the code are exposed and accessible to other modules.

Choose a reason for hiding this comment

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

Granularity:
The test cases are modular and cover different functionalities such as query, get_message, get_labels, and search, which is good practice.

Code Functionality:
The current tests validate the main functions and ensure expected outputs for standard use cases.

Readability & Structure:
The function names are clear and indicate the purpose of the tests. Adding brief comments or docstrings would improve understanding.

Standards Compatibility:
Ensure the code follows PEP 8 guidelines, such as consistent indentation and naming conventions.

Testing:
Improvement: Ensure that each test has assert statements that clearly validate expected outcomes. This will make the tests more formal.
Basic Edge Cases: Consider adding basic checks for situations like empty query results or minimal edge scenarios to ensure better coverage.

Choose a reason for hiding this comment

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

Granularity:
The test script provides manual validation for the main functionalities like querying, fetching, and searching, which is a good start.

Code Functionality:
The use of print statements makes it difficult to validate results in automated environments.

Readability & Structure:
Test outputs are printed directly, making it less structured. Adding assert statements would enhance the readability and reliability of these tests.
Ensure test function names are descriptive to improve clarity.

Standards Compatibility:
Verify that the test code adheres to PEP 8, with consistent indentation and spacing.

Testing:
Improvement: Replace print statements with assert statements to make the test outcomes programmatically verifiable.
Basic Edge Cases: Adding simple tests for no results or incorrect inputs would help improve the robustness of the tests.

@Frankbz Frankbz force-pushed the Gmail/hw3 branch 2 times, most recently from 6062e11 to 060276a Compare November 10, 2024 01:52
Copy link
Collaborator

@kyotov kyotov left a comment

Choose a reason for hiding this comment

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

Major work left.

  • How is each file in your PR connected to the task at hand?
    • You should not have files that are irrelevant to the task.
  • Make sure files that should be .gitignore are in .gitignore
    • E.g. .DS_Store
  • Make sure secrets are not checked in!
  • Make sure there is a clear and separated API!

Address at least the above and then I will look again.

@Frankbz
Copy link
Author

Frankbz commented Dec 7, 2024

new PR: Nightshade14#4

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.

4 participants