Skip to content

Conversation

@austinhuang0131
Copy link
Owner

Pull Request

Summary

Adapt hw1 code, actually call it an adapter as assignment indicated, fix tests, and make CircleCI pass.

Related Issues

Change Type

  • 🐛 Bug fix
  • ✨ Feature / enhancement
  • 💥 Breaking change
  • 📚 Documentation
  • 🔧 Refactor / cleanup
  • ⚡ Performance
  • 🧪 Test improvement
  • 🔨 Build / tooling

Impacted Areas

  • mail_client_api
  • gmail_client_impl
  • Documentation
  • Tests
  • Tooling / CI
  • Other (describe in summary)

Testing

Commands executed

# Example (edit as needed)
uv run pytest src/ tests/
uv run mypy src tests
uv run ruff check .

Results

  • Tests pass
  • Type checks clean
  • Linting clean
  • Manual verification (if applicable)

Quality Checklist

  • Contract boundaries respected; abstractions unchanged unless noted
  • Backward compatibility confirmed or migration documented
  • Security / secrets handled correctly
  • Performance impact acceptable
  • Docs updated for user-facing changes

Notes for Reviewers

@austinhuang0131 austinhuang0131 changed the title Hw1 new Hw1: add FastAPI for interacting with mail client, plus adapter to act like a mail client... version 2 Oct 19, 2025
import os
from typing import Annotated

if os.environ.get("MOCK_CLIENT") == "1":
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't say this is the best way to do this -- you're coupling application code with test code. I would recommend removing test_client.py and using MagicMock to create a mock client in your unit tests instead.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I wouldn't either but I have not found a way to patch it then run with uvicorn in the integration test (which is not supposed to be with production credentials) so unfortunately that's the current compromise. Maybe I'll look into it further later on.

@adithyab-20
Copy link
Collaborator

Overall, nice work! Your peers gave you good feedback and you've made the right changes. The structure of your components is spot on. Your service is now correct as well (needs some minor fixes). Your adapter correctly implements the mail_client_api.Client interface which calls methods on the autogenerated client internally. Well done!

@austinhuang0131 austinhuang0131 merged commit 581af2f into root Oct 23, 2025
5 checks passed
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