Modernize project tooling and packaging#15
Merged
Merged
Conversation
- Replace setup.py/setup.cfg with pyproject.toml using hatchling - Replace flake8 with ruff for linting and formatting - Add pre-commit configuration - Switch from Sphinx to mkdocs with Material theme - Add GitHub Pages docs deployment workflow - Switch CI/CD to use uv and modern action versions - Update deploy workflow to use trusted publishing - Modernize Python code: type hints, f-strings, Google docstrings - Convert README from RST to Markdown - Drop Python 3.8-3.10, require 3.11+ - Remove CodeClimate integration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR modernizes packaging, CI/CD, and docs: removes setuptools/setup.cfg and Sphinx artifacts, adds pyproject.toml/Hatchling and MkDocs, updates GitHub Actions to use astral-sh/setup-uv and uv-based commands, introduces pre-commit/ruff, and modernizes type hints and provider definitions across the library and tests. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
email_normalize/providers.py (1)
34-35: Consider using|instead of^for flag combination.The code uses XOR (
^) to combineRulesflags, but the semantic intent appears to be combining (union) rather than toggling. While the result is identical for distinctenum.auto()values, using OR (|) better expresses the intent of "provider supports both features."💡 Suggested clarification
class Fastmail(MailboxProvider): - Flags = Rules.PLUS_ADDRESSING ^ Rules.LOCAL_PART_AS_HOSTNAME + Flags = Rules.PLUS_ADDRESSING | Rules.LOCAL_PART_AS_HOSTNAME MXDomains = frozenset({'messagingengine.com'})Similarly for
- Flags = Rules.PLUS_ADDRESSING ^ Rules.STRIP_PERIODS + Flags = Rules.PLUS_ADDRESSING | Rules.STRIP_PERIODS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@email_normalize/providers.py` around lines 34 - 35, The Flags assignment uses XOR (^) which suggests toggling but the intent is a union of features; update the Flags expressions to use bitwise OR (|) instead of XOR for clarity (e.g., change Flags = Rules.PLUS_ADDRESSING ^ Rules.LOCAL_PART_AS_HOSTNAME to use |) and make the same change for the Google provider Flags so the code clearly expresses "supports both" rather than toggling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/deploy.yaml:
- Around line 11-12: The workflow's job-level permissions currently only include
id-token: write which prevents actions/checkout@v5 from working; update the
permissions block to include contents: read alongside id-token: write (i.e., add
contents: read under the permissions mapping) so checkout can access the
repository contents while retaining id-token: write for OIDC.
In `@docs/index.md`:
- Around line 3-4: Update the intro sentence for accuracy and readability:
change the description of the project name "email-normalize" to state it targets
Python 3.11+ (not just "Python 3") and hyphenate "mailbox-provider-specific
behaviors", e.g. reword to something like "email-normalize is a Python 3.11+
library that returns a normalized email address, stripping
mailbox-provider-specific behaviors such as plus addressing."
In `@README.md`:
- Around line 3-4: Update the README intro sentence to reflect supported Python
versions and correct hyphenation: change the Python version text from "Python 3"
to "Python 3.11+" and hyphenate "mailbox-provider-specific" in the phrase about
stripping provider behaviors (e.g., "stripping mailbox-provider-specific
behaviors such as 'Plus addressing'") so the first two lines align with Line 35
and use the correct hyphenation.
---
Nitpick comments:
In `@email_normalize/providers.py`:
- Around line 34-35: The Flags assignment uses XOR (^) which suggests toggling
but the intent is a union of features; update the Flags expressions to use
bitwise OR (|) instead of XOR for clarity (e.g., change Flags =
Rules.PLUS_ADDRESSING ^ Rules.LOCAL_PART_AS_HOSTNAME to use |) and make the same
change for the Google provider Flags so the code clearly expresses "supports
both" rather than toggling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34b62087-1461-4841-bc65-340dad35c94a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.codeclimate.yml.editorconfig.github/workflows/deploy.yaml.github/workflows/docs.yaml.github/workflows/testing.yaml.gitignore.pre-commit-config.yamlCONTRIBUTING.mdMANIFEST.inREADME.mdREADME.rstVERSIONdocs/Makefiledocs/_static/css/custom.cssdocs/api.mddocs/conf.pydocs/index.mddocs/index.rstdocs/make.batdocs/mxrecords.rstdocs/normalize.rstdocs/normalizer.rstdocs/requirements.txtdocs/result.rstemail_normalize/__init__.pyemail_normalize/providers.pymkdocs.ymlpyproject.tomlsetup.cfgsetup.pytests/test_normalize.pytests/test_normalizer.py
💤 Files with no reviewable changes (15)
- docs/mxrecords.rst
- docs/Makefile
- docs/result.rst
- docs/requirements.txt
- docs/conf.py
- docs/normalizer.rst
- docs/normalize.rst
- docs/index.rst
- VERSION
- README.rst
- setup.cfg
- docs/_static/css/custom.css
- setup.py
- docs/make.bat
- .codeclimate.yml
- Remove trailing blank line in .gitignore (end-of-file-fixer) - Use | instead of ^ for combining Rules flags in providers - Add contents: read permission to deploy workflow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
setup.py/setup.cfgwithpyproject.tomlusing hatchling build backendactions/checkout@v5,setup-uv@v6,codecov@v5)list[]/tuple[]type hints,str | Noneunions, f-strings, Google-style docstringsfrozenset+typing.ClassVarfor immutable provider class attributesTest plan
--strictCo-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Summary by CodeRabbit
Documentation
Python Support
Build & Tooling