Skip to content

Fix DictsView.dicts instance attr shadowing Table.dicts() method#697

Open
gaoflow wants to merge 2 commits into
petl-developers:masterfrom
gaoflow:fix-fromdicts-dicts-method-shadow
Open

Fix DictsView.dicts instance attr shadowing Table.dicts() method#697
gaoflow wants to merge 2 commits into
petl-developers:masterfrom
gaoflow:fix-fromdicts-dicts-method-shadow

Conversation

@gaoflow

@gaoflow gaoflow commented Jun 25, 2026

Copy link
Copy Markdown

Problem

fromdicts() returns a DictsView (in petl/io/json.py) that stores its
input data as self.dicts. That name clashes with the dicts() method
inherited from Table, which is bound at the class level via
Table.dicts = dicts in petl/util/base.py.

The instance attribute wins in Python's attribute-lookup order, so any
.dicts() call on the result raises:

TypeError: 'DictsView' object is not callable

Minimal reproducer (reported in #643):

import petl as etl
dummy = etl.dummytable()
table = etl.fromdicts(dummy.dicts())
table.dicts()   # TypeError: 'DictsView' object is not callable

Fix

Rename the internal attribute self.dictsself._dicts in both
DictsView and DictsGeneratorView, consistent with the existing
self._header convention. Five call-sites updated (two in
__init__, one in DictsView.__iter__, two in
DictsGeneratorView.__iter__ / _determine_header).

A regression test is added to petl/test/io/test_json.py.

Fixes #643.


This pull request was prepared with the assistance of AI, under my direction and review.

DictsView (returned by fromdicts()) stored its input data as
self.dicts, which shadowed the dicts() method inherited from Table.
Calling .dicts() on a fromdicts() result raised TypeError because
Python found the instance attribute (the raw dicts data) instead of
the method.

Rename the internal attribute to self._dicts throughout DictsView and
DictsGeneratorView, consistent with the existing _header convention.

Fixes petl-developers#643.
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Fix fromdicts() DictsView attribute shadowing inherited Table.dicts()
🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

Description

• Rename DictsView internal storage from dicts to _dicts to avoid method shadowing.
• Ensure .dicts() on fromdicts() results returns row dictionaries instead of raising TypeError.
• Add regression test covering the .dicts() call path for DictsView.
Diagram

graph TD
  A["User code"] --> B["etl.fromdicts()"] --> C["DictsView / DictsGeneratorView"] --> D["iterdicts()"]
  C --> E["Table.dicts() method"]
  subgraph Legend
    direction LR
    _fn["Function"] ~~~ _cls["Class/View"]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Rename Table.dicts() to a less collision-prone name
  • ➕ Eliminates a broad class of potential name collisions with instance attributes named dicts
  • ➖ Breaking API change across the library and downstream users
  • ➖ Requires deprecation cycle and widespread updates
2. Convert DictsView storage to a property with a different backing name
  • ➕ Keeps readable attribute access while avoiding collisions (e.g., dicts property backed by _dicts)
  • ➖ Still risks confusion with the method name dicts()
  • ➖ Adds unnecessary indirection for an internal-only field

Recommendation: Proceed with the PR’s approach: renaming the internal attribute to _dicts is the smallest, safest change, consistent with existing private naming (e.g., _header) and avoids any public API churn. Alternatives either introduce breaking changes (renaming Table.dicts()) or add complexity without improving clarity (property indirection).

Files changed (2) +21 / -5

Bug fix (1) +5 / -5
json.pyAvoid shadowing Table.dicts() by renaming DictsView storage to '_dicts' +5/-5

Avoid shadowing Table.dicts() by renaming DictsView storage to '_dicts'

• Changes DictsView and DictsGeneratorView to store input data in 'self._dicts' instead of 'self.dicts'. Updates iteration and header-determination code paths to reference '_dicts', preventing '.dicts()' from being shadowed by an instance attribute.

petl/io/json.py

Tests (1) +16 / -0
test_json.pyAdd regression test for calling '.dicts()' on fromdicts() results +16/-0

Add regression test for calling '.dicts()' on fromdicts() results

• Adds a test ensuring 'fromdicts(...).dicts()' returns the expected row dictionaries and does not raise 'TypeError'. Covers the specific regression caused by attribute/method name collision.

petl/test/io/test_json.py

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 25, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. test_fromdicts_dicts_method lacks dummytable ✓ Resolved 📎 Requirement gap ≡ Correctness
Description
The new regression test calls fromdicts() with a literal list of dicts, but it does not reproduce
the reported sequence dummy = etl.dummytable(); table = etl.fromdicts(dummy.dicts()). This may
miss regressions specific to consuming dummy.dicts() (a generator/view) and does not meet the
checklist’s explicit success criteria.
Code

petl/test/io/test_json.py[R344-357]

+def test_fromdicts_dicts_method():
+    # fromdicts() returns a DictsView whose internal data attribute was named
+    # 'dicts', shadowing the inherited Table.dicts() method.  Calling .dicts()
+    # on the result should return row dicts, not raise TypeError.
+    data = [{'foo': 'a', 'bar': 1},
+            {'foo': 'b', 'bar': 2},
+            {'foo': 'c', 'bar': 2}]
+    actual = fromdicts(data, header=['foo', 'bar'])
+    result = list(actual.dicts())
+    assert result == [
+        {'foo': 'a', 'bar': 1},
+        {'foo': 'b', 'bar': 2},
+        {'foo': 'c', 'bar': 2},
+    ]
Evidence
PR Compliance ID 2 requires a test that uses etl.dummytable() and etl.fromdicts(dummy.dicts()).
The added test instead constructs data = [...] and calls actual = fromdicts(data, ...), which
does not match the required reproducer steps.

Add regression coverage for DictsView/method name collision on dicts
petl/test/io/test_json.py[344-357]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The regression test does not follow the required reproducer sequence (`dummy = etl.dummytable(); table = etl.fromdicts(dummy.dicts()); table.dicts()`), so it may not prevent reintroduction of the specific collision scenario described in the checklist.
## Issue Context
Checklist PR Compliance ID 2 explicitly requires using `etl.dummytable()` and passing `dummy.dicts()` into `etl.fromdicts(...)`, then asserting `table.dicts()` is callable and yields dict rows.
## Fix Focus Areas
- petl/test/io/test_json.py[344-357]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@juarezr juarezr added this to the v2.0 milestone Jun 26, 2026
@juarezr juarezr added the Bug It must work in all situations, but this failed label Jun 26, 2026
@gaoflow

gaoflow commented Jun 26, 2026

Copy link
Copy Markdown
Author

Thanks, that is a good catch. I pushed 4ba9c84 to make the regression test follow the reported path exactly:

dummy = dummytable(numrows=3, seed=42)
actual = fromdicts(dummy.dicts())
list(actual.dicts())

Local verification:

uv run --with pytest --with pytest-cov --with-editable . python -m pytest petl/test/io/test_json.py::test_fromdicts_dicts_method -q
uv run --with pytest --with pytest-cov --with-editable . python -m pytest petl/test/io/test_json.py -q
uv run --with ruff ruff check petl/test/io/test_json.py petl/io/json.py
git diff --check

I also checked the current red 3.8/3.9 CI jobs. They fail before pytest runs, while building the package via python setup.py sdist bdist_wheel, with AttributeError: ignore_egg_info_in_manifest from the setuptools/setuptools_scm integration in the runner environment. That does not look related to this test/code path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug It must work in all situations, but this failed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dicts attribute of DictsView conflicts with util.base.dicts() method

2 participants