Simplify N802 suppression for common.py
#2880
Merged
Conversation
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
- simplify N802 handling - use `isinstance` instead of `type`
Summary by CodeRabbit
WalkthroughAdded per-file Ruff ignores in pyproject.toml and removed corresponding inline noqa comments from tests/common.py. No functional code changes. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (6)📓 Common learnings📚 Learning: 2025-07-05T20:26:31.219ZApplied to files:
📚 Learning: 2025-06-16T15:48:33.931ZApplied to files:
📚 Learning: 2025-06-15T15:10:20.360ZApplied to files:
📚 Learning: 2025-06-16T11:17:34.671ZApplied to files:
📚 Learning: 2025-06-24T18:25:25.165ZApplied to files:
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2880 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 294 294
Lines 17458 17458
Branches 2263 2263
=========================================
Hits 17458 17458 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/common.py (4)
43-47: Fix impossible default_language length checkThe chained comparison
2 > len(...) > 6is unsatisfiable. Use a negated range check.- and 2 > len(test_class.default_language) > 6 + and not (2 <= len(test_class.default_language) <= 6)
79-87: Prefer isinstance over issubclass(obj.class, …)This aligns with the PR goal and reads clearer.
- if args and issubclass(args[0].__class__, HolidayBase): + if args and isinstance(args[0], HolidayBase): @@ - self.assertTrue( - issubclass(instance.__class__, HolidayBase), + self.assertTrue( + isinstance(instance, HolidayBase), f"The `self.{instance_name}` must be a `HolidayBase` subclass.", ) @@ - self.assertTrue( - issubclass(holidays.__class__, HolidayBase), + self.assertTrue( + isinstance(holidays, HolidayBase), "`holidays` object must be a subclass of `HolidayBase`", )Also applies to: 119-123
192-197: Guard date parsing to avoid unintended exceptions
parse(arg)in the boolean expression can raise on non-strings or unparsable values, bypassing the intended ValueError path.- elif isinstance(arg, date) or parse(arg): # Exact date check. + elif isinstance(arg, date) or (isinstance(arg, str) and self._is_date_string(arg)): for dt in items: self.assertIn(name, holidays.get_list(dt), dt) else: raise ValueError(f"The {arg} wasn't caught by `assertHolidayName()`")- elif isinstance(arg, date) or parse(arg): # Exact date check. + elif isinstance(arg, date) or (isinstance(arg, str) and self._is_date_string(arg)): for dt in items: self.assertNotIn(name, holidays.get_list(dt), dt) else: raise ValueError(f"The {arg} wasn't caught by `assertNoHolidayName()`")Add this helper within the same class to keep behavior centralized:
def _is_date_string(self, s: str) -> bool: try: parse(s) return True except Exception: return FalseAlso applies to: 299-304
335-336: Pass years as int, not strMinor type nit for clarity; HolidayBase accepts ints here.
- years=localized_holidays[0][0].split("-")[0], + years=int(localized_holidays[0][0].split("-")[0]),tests/test_utils.py (1)
144-144: Unify N802 handling (optional)Either rename to snake_case and drop inline noqa, or add a per-file ignore in pyproject as suggested.
Option A (rename):
-class TestListLocalizedEntities(unittest.TestCase): - def assertLocalizedEntities(self, localized_entities, supported_entities): # noqa: N802 +class TestListLocalizedEntities(unittest.TestCase): + def assert_localized_entities(self, localized_entities, supported_entities): @@ - def test_localized_countries(self): - self.assertLocalizedEntities( + def test_localized_countries(self): + self.assert_localized_entities( @@ - def test_localized_financial(self): - self.assertLocalizedEntities( + def test_localized_financial(self): + self.assert_localized_entities(Option B (keep name, move suppression): add
lint.per-file-ignores."tests/test_utils.py" = [ "N802" ]in pyproject.toml and remove# noqa: N802here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
pyproject.toml(1 hunks)tests/common.py(13 hunks)tests/test_utils.py(2 hunks)
🧰 Additional context used
🧠 Learnings (21)
📚 Learning: 2025-07-05T20:26:31.219Z
Learnt from: KJhellico
PR: vacanza/holidays#2702
File: holidays/countries/__init__.py:65-69
Timestamp: 2025-07-05T20:26:31.219Z
Learning: The vacanza/holidays project uses ruff as the primary linter for code checking and linting. Flake8 suppression comments are not needed since the project defers to ruff's rules, and existing `ruff: noqa` comments should be sufficient for handling linting warnings.
Applied to files:
pyproject.toml
📚 Learning: 2025-06-16T15:48:33.931Z
Learnt from: PPsyrius
PR: vacanza/holidays#2615
File: tests/countries/test_anguilla.py:20-24
Timestamp: 2025-06-16T15:48:33.931Z
Learning: The holidays project uses `make pre-commit` test which includes `ruff` as the primary linter. If `ruff` passes, then Pylint warnings that don't appear in `ruff` are considered acceptable and can be ignored.
Applied to files:
pyproject.toml
📚 Learning: 2025-06-16T11:17:34.671Z
Learnt from: PPsyrius
PR: vacanza/holidays#2637
File: holidays/countries/singapore.py:49-50
Timestamp: 2025-06-16T11:17:34.671Z
Learning: For the vacanza/holidays project, defer to ruff's line length rules rather than pylint's C0301 warnings. If ruff passes in pre-commit tests, line length is acceptable regardless of pylint warnings about exceeding 100 characters.
Applied to files:
pyproject.toml
📚 Learning: 2025-06-24T18:25:25.165Z
Learnt from: PPsyrius
PR: vacanza/holidays#2601
File: holidays/countries/mongolia.py:25-30
Timestamp: 2025-06-24T18:25:25.165Z
Learning: In the holidays project, reference URLs in docstrings/comments that exceed 100 characters are acceptable if they pass the pre-commit tests, even if Pylint flags them as line length violations.
Applied to files:
pyproject.toml
📚 Learning: 2025-04-05T04:47:27.213Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:52-64
Timestamp: 2025-04-05T04:47:27.213Z
Learning: For holiday tests in the vacanza/holidays project, structure tests by individual holidays rather than by years. Each test method should focus on a specific holiday and test it across multiple years (from start_year through 2050) using helper methods like `assertHolidayName`. For fixed holidays, use generators like `(f"{year}-01-01" for year in range(1991, 2051))`. For movable holidays, specify individual dates for specific years followed by a range check.
Applied to files:
tests/test_utils.pytests/common.py
📚 Learning: 2025-05-06T21:07:11.577Z
Learnt from: KJhellico
PR: vacanza/holidays#2530
File: tests/countries/test_andorra.py:23-28
Timestamp: 2025-05-06T21:07:11.577Z
Learning: In the holidays project, test methods for country holidays follow a standard form where year ranges are explicitly recreated in each test method rather than being stored as class variables, to maintain consistency across all country tests.
Applied to files:
tests/test_utils.py
📚 Learning: 2025-08-19T19:47:21.735Z
Learnt from: KJhellico
PR: vacanza/holidays#2829
File: tests/countries/test_canada.py:291-296
Timestamp: 2025-08-19T19:47:21.735Z
Learning: In the holidays library, HolidayBase objects automatically populate years on-demand when expand=True (the default). When checking dates from years not initially in the years range, those years are automatically populated via the logic at line 646 in holiday_base.py: "if self.expand and dt.year not in self.years:". This means tests can check dates outside the initial year range without needing separate holiday instances.
Applied to files:
tests/test_utils.py
📚 Learning: 2025-08-12T17:16:54.497Z
Learnt from: PPsyrius
PR: vacanza/holidays#2794
File: tests/calendars/test_julian.py:35-36
Timestamp: 2025-08-12T17:16:54.497Z
Learning: In the vacanza/holidays project calendar tests (Thai, Ethiopian, Julian, etc.), the established testing pattern for validation methods is to use simple for loops like `for year in known_data_dict:` followed by `self.assertEqual(expected, actual)` without using unittest's subTest feature. This pattern is consistently maintained across all calendar test files.
Applied to files:
tests/test_utils.pytests/common.py
📚 Learning: 2025-04-05T06:49:06.217Z
Learnt from: PPsyrius
PR: vacanza/holidays#2386
File: tests/countries/test_nepal.py:499-536
Timestamp: 2025-04-05T06:49:06.217Z
Learning: In the holidays project, test files follow a dual testing approach: individual methods test specific holidays across multiple years, while comprehensive year-specific tests (e.g., `test_2025`) verify all holidays for a specific year in a single assertion. Both approaches serve different testing purposes and complement each other.
Applied to files:
tests/test_utils.pytests/common.py
📚 Learning: 2025-06-18T10:38:45.677Z
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: tests/countries/test_france.py:70-73
Timestamp: 2025-06-18T10:38:45.677Z
Learning: In the holidays library, the `assertDeprecatedSubdivisions` method uses a generic hard-coded message ("This subdivision is deprecated and will be removed") for all deprecated subdivision tests. This is the library-wide standard pattern, not specific to individual countries.
Applied to files:
tests/common.py
📚 Learning: 2025-07-09T21:16:35.145Z
Learnt from: KJhellico
PR: vacanza/holidays#2623
File: tests/countries/test_christmas_island.py:136-146
Timestamp: 2025-07-09T21:16:35.145Z
Learning: In Christmas Island's holiday implementation, the test_christmas_day method cannot use assertNoNonObservedHoliday because in some years observed Christmas Day overlaps with Boxing Day when both holidays are moved due to weekend conflicts, causing the standard non-observed holiday check to fail inappropriately.
Applied to files:
tests/common.py
📚 Learning: 2025-04-02T18:38:35.164Z
Learnt from: KJhellico
PR: vacanza/holidays#2398
File: tests/countries/test_guinea.py:237-239
Timestamp: 2025-04-02T18:38:35.164Z
Learning: In the vacanza/holidays project, the method assertLocalizedHolidays in country test files should be called with positional parameters rather than named parameters to maintain consistency with the rest of the codebase.
Applied to files:
tests/common.py
📚 Learning: 2025-07-24T15:21:31.632Z
Learnt from: PPsyrius
PR: vacanza/holidays#2750
File: tests/countries/test_germany.py:46-46
Timestamp: 2025-07-24T15:21:31.632Z
Learning: In the holidays project test files, the standard method name for testing the absence of holidays is `test_no_holidays`, not more descriptive names like `test_no_holidays_before_1990`. This is a consistent naming convention across country test files like France and Germany.
Applied to files:
tests/common.py
📚 Learning: 2025-08-30T12:49:19.654Z
Learnt from: KJhellico
PR: vacanza/holidays#2834
File: tests/financial/test_national_stock_exchange_of_india.py:342-360
Timestamp: 2025-08-30T12:49:19.654Z
Learning: The assertLocalizedHolidays method in the vacanza/holidays project requires a complete list of all holidays for the specific year being tested, not just a subset. When testing localization, all holidays from that year must be included in the assertion.
Applied to files:
tests/common.py
📚 Learning: 2025-08-25T04:28:02.061Z
Learnt from: PPsyrius
PR: vacanza/holidays#2848
File: tests/countries/test_somalia.py:44-127
Timestamp: 2025-08-25T04:28:02.061Z
Learning: In the holidays library, Islamic holiday tests use `self.no_estimated_holidays = Country(years=years, islamic_show_estimated=False)` as the library-wide standard approach to simplify test cases. This pattern is intentional and preferred over testing estimated labels.
Applied to files:
tests/common.py
📚 Learning: 2025-03-23T16:59:25.794Z
Learnt from: PPsyrius
PR: vacanza/holidays#2362
File: tests/test_ical.py:31-40
Timestamp: 2025-03-23T16:59:25.794Z
Learning: In the holidays library's `TestIcalExporter` class, initializing `ICalExporter` with `None` is intentional. Using a mock object instead breaks tests because the constructor performs attribute validation with `getattr(self.holidays, "language", None)` which returns a mock with a mock object instead of `None`, causing validation failures. The approach isolates file operation tests from iCal generation logic.
Applied to files:
tests/common.py
📚 Learning: 2025-08-30T12:52:58.513Z
Learnt from: KJhellico
PR: vacanza/holidays#2834
File: tests/financial/test_national_stock_exchange_of_india.py:342-360
Timestamp: 2025-08-30T12:52:58.513Z
Learning: In the NSE holidays implementation, assertLocalizedHolidays should only include holidays that are actually observed (trading holidays), not holidays that fall on weekends and are excluded by the observed_rule. For example, Eid al-Fitr 2023 falls on Saturday and is correctly excluded from localization tests.
Applied to files:
tests/common.py
📚 Learning: 2025-04-05T04:29:38.042Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:85-86
Timestamp: 2025-04-05T04:29:38.042Z
Learning: For testing holiday implementations in the vacanza/holidays repository, recommend using `from tests.common import CommonCountryTests` as the base class instead of directly using `unittest.TestCase` to maintain consistency with project conventions and leverage common test utilities.
Applied to files:
tests/common.py
📚 Learning: 2025-04-05T04:50:40.752Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:31-49
Timestamp: 2025-04-05T04:50:40.752Z
Learning: For Turkmenistan holiday tests, use this class structure: `class TestTurkmenistan(CommonCountryTests, TestCase)` with imports `from unittest import TestCase`, `from holidays.countries import Turkmenistan, TM, TKM`, and `from tests.common import CommonCountryTests`. Ensure to call `super().setUp()` in the setUp method.
Applied to files:
tests/common.py
📚 Learning: 2025-06-18T17:01:58.067Z
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: tests/countries/test_wallis_and_futuna.py:19-23
Timestamp: 2025-06-18T17:01:58.067Z
Learning: In the vacanza/holidays repository, the standard pattern for test class setUpClass methods is `super().setUpClass(HolidayClass)` or `super().setUpClass(HolidayClass, years=...)` where HolidayClass is passed as the first argument. This is the correct signature that matches the CommonCountryTests.setUpClass method which accepts test_class as the first parameter after cls. Pylint warnings about parameter count mismatch are false positives when comparing against TestCase.setUpClass instead of the immediate parent CommonCountryTests.setUpClass.
Applied to files:
tests/common.py
📚 Learning: 2025-04-05T04:33:53.254Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:52-64
Timestamp: 2025-04-05T04:33:53.254Z
Learning: For Turkmenistan holiday tests, recommend using CommonCountryTests as the base class rather than unittest.TestCase to follow project conventions, be consistent with other country test files, and gain access to common test utilities.
Applied to files:
tests/common.py
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Python 3.13 on windows-latest
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_utils.py (2)
149-154: Micro: avoid redundant Path()
locale_diris already a Path.- for path in Path(locale_dir).rglob(f"{entity_code}.mo") + for path in locale_dir.rglob(f"{entity_code}.mo")
57-61: Duplicate assertion callTwo identical
country_holidays("US", subdiv="XXXX")checks. Keep one.- self.assertRaises(NotImplementedError, lambda: country_holidays("US", subdiv="XXXX")) - self.assertRaises(NotImplementedError, lambda: country_holidays("US", subdiv="XXXX")) + self.assertRaises(NotImplementedError, lambda: country_holidays("US", subdiv="XXXX"))
♻️ Duplicate comments (1)
tests/test_utils.py (1)
115-116: Prefer assertIsInstance over type equalityYields clearer failures and allows subclassing. Apply to both loops.
- self.assertEqual(type(dt), date) + self.assertIsInstance(dt, date) ... - self.assertEqual(type(dt), date) + self.assertIsInstance(dt, date)Also applies to: 137-139
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pyproject.toml(1 hunks)tests/test_utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: KJhellico
PR: vacanza/holidays#2702
File: holidays/countries/__init__.py:65-69
Timestamp: 2025-07-05T20:26:31.219Z
Learning: The vacanza/holidays project uses ruff as the primary linter for code checking and linting. Flake8 suppression comments are not needed since the project defers to ruff's rules, and existing `ruff: noqa` comments should be sufficient for handling linting warnings.
Learnt from: PPsyrius
PR: vacanza/holidays#2615
File: tests/countries/test_anguilla.py:20-24
Timestamp: 2025-06-16T15:48:33.931Z
Learning: The holidays project uses `make pre-commit` test which includes `ruff` as the primary linter. If `ruff` passes, then Pylint warnings that don't appear in `ruff` are considered acceptable and can be ignored.
📚 Learning: 2025-07-05T20:26:31.219Z
Learnt from: KJhellico
PR: vacanza/holidays#2702
File: holidays/countries/__init__.py:65-69
Timestamp: 2025-07-05T20:26:31.219Z
Learning: The vacanza/holidays project uses ruff as the primary linter for code checking and linting. Flake8 suppression comments are not needed since the project defers to ruff's rules, and existing `ruff: noqa` comments should be sufficient for handling linting warnings.
Applied to files:
pyproject.toml
📚 Learning: 2025-06-16T15:48:33.931Z
Learnt from: PPsyrius
PR: vacanza/holidays#2615
File: tests/countries/test_anguilla.py:20-24
Timestamp: 2025-06-16T15:48:33.931Z
Learning: The holidays project uses `make pre-commit` test which includes `ruff` as the primary linter. If `ruff` passes, then Pylint warnings that don't appear in `ruff` are considered acceptable and can be ignored.
Applied to files:
pyproject.toml
📚 Learning: 2025-06-15T15:10:20.360Z
Learnt from: KJhellico
PR: vacanza/holidays#2632
File: holidays/countries/solomon_islands.py:28-54
Timestamp: 2025-06-15T15:10:20.360Z
Learning: The vacanza/holidays project uses ruff with line-length = 99 characters (configured in pyproject.toml), not the standard 80 or 100 character limits. When pre-commit checks pass, the formatting is compliant with the project's specific ruff configuration.
Applied to files:
pyproject.toml
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Python 3.13 on windows-latest
🔇 Additional comments (1)
tests/test_utils.py (1)
144-144: Good: moved N802 suppression out of the fileInline noqa is gone; helper name stays as-is under per-file ignore. Consistent with ruff-first policy.
common.py
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Proposed change
Update common.py and test_utils.py
Type of change
holidaysfunctionality in general)Checklist
make checklocally; all checks and tests passed.