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

Mini Task - mAIgic Team 3 - Gmail Communication #13

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

Conversation

TechPertz
Copy link

This is the new PR for the gmail communication mini-task. Although numerous function and files were added beyond this stage, this PR follows the mini task that was initially assigned. This code establishes communication with the gmail server as per the requirements.

Members:
Reet Nandy [[email protected]]
Anushka Tawte [[email protected]]
Rafael [[email protected]]

@Nightshade14
Copy link

If using uv, it won't allow leaving __init__.py files as blank files unless excluded from the linting rules.

@Nightshade14
Copy link

PR Review according to the provided template / guidelines.

Change Granularity:

  • The commits are well defined and clearly indicate the development workflow.

Code Functionality:

  • The code functions as expected.
  • It is meant to fetch Gmail emails and it does so.

Readability & Structure:

  • The variable names, class names and functions name are well defined, intuitive and descriptive.
  • Suggestion: Add docstrings to class methods as well.
  • Optional suggestion: Rename the files as such that the files that are not meant to be used directly by the end user, must start with underscore in their names. It serves as a passive warning to the user. As in python, one can't strictly restrict a user in accessing any files. So, a nomenclature / standard would be appreciated.

Standards Compatibility:

  • Suggestion: The codebase does not indicate any use of linting, static type checking or of a package and project manager i.e. Ruff, Mypy, uv . Recommend the use of such tools to ensure reliability, robustness and replication.
  • Suggestion: The codebase does not indicate any use of Continuous Integration tools like CircleCI. Recommend the use of CI to ensure project robustness.

Testing:

  • The tests cover both i.e. expected and edge scenarios.
  • The tests are well-structured and readable.

@Elisha-Edme
Copy link

Change Granularity:

I think the commits have good messages and none are overloaded

Code Functionality:

The code works as expected. The files are named appropriately for the functionality they provide

Readability:

There are barely any comments, and the comments aren't that descriptive
I don't think you need the init.py in your tests folder since it's not a module the user will be using
I think all the files except api.py should have underscores in the name to show that they are hidden

Standards Compatibility:

I think it complies with standard SWE practices, looks good to me!

Testing:

The tests cover all edgecases, and the naming is helpful

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