Fix cache/complement/index/skip attributes shadowing Table methods

[gaoflow]

Follow-up to #697 and #704, finishing the same collision class.

The class

Table gets most of its fluent API from module-level assignments
(Table.cache = cache, Table.complement = complement, Table.skip = skip)
plus the methods it inherits from IterContainer (index, min, list, ...) —
263 callables in total. Views then store their constructor arguments as instance
attributes of the same name. Python resolves the instance attribute first, so the
method is gone and calling it raises TypeError:

>>> import petl as etl
>>> etl.sort([['foo'], ['a']], 'foo').cache()
TypeError: 'bool' object is not callable
>>> etl.select([['foo'], ['a']], lambda row: True).complement([['foo']])
TypeError: 'bool' object is not callable
>>> etl.addfield([['foo'], ['a']], 'bar', 1).index(('a', 1))
TypeError: 'NoneType' object is not callable

5498411 (#555) hit this on header, #697 on dicts, #704 on the remaining
thirteen header sites. I went back over the whole surface rather than the one
call I tripped over, by intersecting the 263-name API with every self.<name> =
assignment in a Table subclass. Twelve views were still affected:

method	view	file
cache()	HashJoinView	transform/hashjoins.py
cache()	HashLeftJoinView	transform/hashjoins.py
cache()	HashRightJoinView	transform/hashjoins.py
cache()	SortView	transform/sorts.py
cache()	CacheView	util/materialise.py
complement()	SearchView	transform/regex.py
complement()	RowSelectView	transform/selects.py
complement()	FieldSelectView	transform/selects.py
index()	AddFieldView	transform/basics.py
index()	MoveFieldView	transform/basics.py
skip()	AvroView	io/avro.py
skip()	BcolzView	io/bcolz.py

All twelve raise today; all twelve pass after the change, and the scan now comes
back empty.

The fix

Rename the stored attribute with a leading underscore, the convention 5498411
introduced for header and #697 followed for dicts.

The compatibility question that came up on #697 does not arise here. .dicts was
the caller's own input data, so that PR kept it readable through a property. These
twelve are internal flags and offsets (cache=True, complement=False, index=0,
skip=0) — nothing in the docs, examples or tests reads them back off a view, and
grep finds no reader outside the class that owns them. So a plain rename, no shim.

I also looked at fixing this at the root, in Table itself: a __setattr__ that
refuses to store an attribute shadowing a method would catch the whole class
permanently. It would put a Python-level call on every attribute write in every
view for a problem that only bites at construction time, and it would break any
downstream subclass doing the same thing. Not worth it — a test can do the same
job for free.

Tests

test_method_shadow.py grows from the thirteen header cases to cover the four
new names: one test per view calling the method that used to raise, plus tests
that the renamed attributes still carry their options (cache=False really
disables caching, complement=True really inverts the selection, index=0 really
positions the field, skip=n really skips). fromavro/frombcolz are driven
without fastavro and bcolz installed, since neither is needed to reach the
skipping code.

test_no_shadowed_api_names is the one that matters: it walks the package,
finds every Table subclass, and fails if any of them assigns an instance
attribute named after an API method — reporting file, line, class and name. It
reproduces all twelve sites on master and would have caught #555, #643 and #704
as well. A deliberate class-level override (DictsView.dicts, the property #697
added) is not flagged.

586 pass, 13 skip; pytest --doctest-modules 739 pass, 21 skip. I mutated each
renamed read in both directions — leaving a site shadowed, and dropping or
inverting the option the attribute carries — and every mutant is caught.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix Table method shadowing by cache/complement/index/skip view attributes

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Rename view instance attributes that shadow Table fluent methods (cache/complement/index/skip).
• Update affected join/sort/select/regex/materialise/avro/bcolz views to use underscored fields.
• Add regression tests plus an AST scan to prevent future API-name collisions in CI.

Diagram

graph TD
  U(["User code"]) --> T["Table fluent methods"] --> V["View subclasses"] --> A[("Underscored attrs")] --> I["Iterators use attrs"] --> R["Methods callable"]
  S[["test_method_shadow.py"]] --> C{"Shadowed API name?"} --> F["CI fails"]
  C -->|"pass"| R

  subgraph Legend
    direction LR
    _u(["Caller"]) ~~~ _p["Component"] ~~~ _d[("Internal state")] ~~~ _t[["Test"]] ~~~ _c{"Check"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Guard against shadowing in Table.__setattr__

• ➕ Would prevent the entire class of bugs at runtime for all downstream views
• ➕ Immediate error at construction time with a clear message
• ➖ Adds overhead to every attribute write on every Table subclass instance
• ➖ High compatibility risk for downstream subclasses that intentionally use those names
• ➖ Does not replace the need for test coverage across optional modules

2. Replace fluent API assignments with descriptors/properties

• ➕ Could separate internal state names from method names more robustly
• ➕ May improve discoverability/documentation of the fluent surface
• ➖ Large refactor across a wide API surface (hundreds of callables)
• ➖ High regression risk and likely backward-compatibility concerns

Recommendation: Keep the PR’s approach: rename the conflicting instance attributes to underscored names and enforce the invariant with a source-scanning test. It fixes the concrete failures with minimal behavior change and low compatibility risk, while the AST-based test provides durable protection without introducing runtime overhead or breaking downstream subclasses.

Files changed (9) +282 / -38

Bug fix (8) +33 / -33

avro.pyAvoid shadowing Table.skip() in AvroView +2/-2

Avoid shadowing Table.skip() in AvroView

• Rename the constructor-stored skip offset from self.skip to self._skip. Update row-reading logic to reference the underscored attribute so Table.skip() remains callable on AvroView instances.

petl/io/avro.py

bcolz.pyAvoid shadowing Table.skip() in BcolzView +3/-3

Avoid shadowing Table.skip() in BcolzView

• Rename the stored skip parameter to self._skip. Update ctable iteration/where calls to pass the renamed value, preventing Table.skip() from being masked.

petl/io/bcolz.py

basics.pyAvoid shadowing IterContainer.index() in AddFieldView/MoveFieldView +4/-4

Avoid shadowing IterContainer.index() in AddFieldView/MoveFieldView

• Rename stored field position from self.index to self._index in AddFieldView and MoveFieldView. Update iterator logic to use the underscored value so the inherited .index() method remains accessible.

petl/transform/basics.py

hashjoins.pyAvoid shadowing Table.cache() in hash join views +6/-6

Avoid shadowing Table.cache() in hash join views

• Rename the join lookup caching flag from self.cache to self._cache across HashJoinView, HashLeftJoinView, and HashRightJoinView. Update __iter__ logic to use the renamed flag so Table.cache() is not hidden.

petl/transform/hashjoins.py

regex.pyAvoid shadowing Table.complement() in SearchView +2/-2

Avoid shadowing Table.complement() in SearchView

• Rename the complement-selection flag from self.complement to self._complement. Update itersearch invocation accordingly so the fluent .complement() method remains callable.

petl/transform/regex.py

selects.pyAvoid shadowing Table.complement() in select views +4/-4

Avoid shadowing Table.complement() in select views

• Rename stored complement flags to self._complement in RowSelectView and FieldSelectView. Update iterator calls so Table.complement() is not masked by instance state.

petl/transform/selects.py

sorts.pyAvoid shadowing Table.cache() in SortView +5/-5

Avoid shadowing Table.cache() in SortView

• Rename SortView’s caching toggle from self.cache to self._cache and update cache-path checks. This keeps SortView.cache() callable via the fluent API while preserving existing caching behavior.

petl/transform/sorts.py

materialise.pyAvoid shadowing Table.cache() in CacheView internal buffer +7/-7

Avoid shadowing Table.cache() in CacheView internal buffer

• Rename CacheView’s internal row buffer from self.cache to self._cache and adjust clear/iteration logic. Prevents the internal list from hiding the Table.cache() fluent method.

petl/util/materialise.py

Tests (1) +249 / -5

test_method_shadow.pyAdd regression tests and an AST guard for API-name shadowing +249/-5

Add regression tests and an AST guard for API-name shadowing

• Expand regression coverage to cache/complement/index/skip collisions with per-view tests that previously raised TypeError. Add behavioral assertions that the renamed internal options still work, plus an AST-based scan that fails if any Table subclass assigns self.<api_method_name> without a deliberate class-level override.

petl/test/test_method_shadow.py

[coveralls]

Coverage Report for CI Build 30894017993

Coverage increased (+0.2%) to 91.864%

Details

• Coverage increased (+0.2%) from the base build.
• Patch coverage: 8 uncovered changes across 2 files (175 of 183 lines covered, 95.63%).
• No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
petl/test/test_method_shadow.py	153	146	95.42%
petl/io/bcolz.py	3	2	66.67%
Total (9 files)	183	175	95.63%

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	15254
Covered Lines:	14013
Line Coverage:	91.86%
Coverage Strength:	0.92 hits per line

---

💛 - Coveralls

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 dicts attribute of DictsView conflicts with util.base.dicts() method

1. Test scan traverses tests 🐞 Bug ☼ Reliability

Description

test_no_shadowed_api_names() attempts to skip the petl/test directory, but because it doesn’t prune
os.walk() traversal it still parses petl/test/* subdirectories. This makes the scan depend on
test-only modules and can break or misbehave under non-UTF8 default encodings when reading test
sources containing non-ASCII characters.

Code

petl/test/test_method_shadow.py[R316-318]

+    for dirpath, _, filenames in os.walk(root):
+        if os.path.basename(dirpath) == 'test':
+            continue

Evidence

The scan’s skip logic only checks the current dirpath basename and does not stop traversal into
petl/test children, and the code reads sources using open(path) without specifying/deriving
encoding. There are UTF-8-encoded test files under petl/test/... containing non-ASCII characters,
which the scan will still read and parse.

petl/test/test_method_shadow.py[314-328]
petl/test/io/test_xlsx.py[1-31]

Agent prompt

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

### Issue description
`test_no_shadowed_api_names()` uses `os.walk()` and `continue` to skip a directory named `test`, but this does not prevent `os.walk()` from descending into `petl/test/...`. As a result, the scan parses test modules too, and reads them with `open(path)` using the platform default encoding.

### Issue Context
Some `petl/test/...` files contain non-ASCII source (e.g., `u"é"`). Reading them via `open(path)` without honoring the source encoding cookie makes the test less portable and can fail under ASCII (or otherwise incompatible) locales.

### Fix Focus Areas
- petl/test/test_method_shadow.py[314-328]

### What to change
- Prune traversal by capturing `dirnames` from `os.walk()` and removing `test` (and potentially `__pycache__`) in-place so subdirectories under `petl/test` aren’t visited.
- Use an encoding-aware opener for Python source (e.g., `tokenize.open` on Py3, or a small helper that respects `# -*- coding: ... -*-`) instead of `open(path)`.

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

---

2. Dict-order dependent test 🐞 Bug ☼ Reliability

Description

test_fromavro_skip() constructs plain dict records but AvroView._map_row_from() uses
tuple(record.values()) on Python 3, so output column order follows mapping iteration order rather
than the provided header. This makes the new test depend on dict-order guarantees (or implementation
details) instead of explicitly modeling schema/header ordering.

Code

petl/test/test_method_shadow.py[R214-217]

+    records = [{'foo': 'a', 'bar': 1}, {'foo': 'b', 'bar': 2},
+               {'foo': 'c', 'bar': 3}]
+    eq_([('c', 3)],
+        list(view._read_rows_from(records, ('foo', 'bar'))))

Evidence

The new test feeds dicts into _read_rows_from() and expects a tuple ordered like the header, but
_map_row_from() explicitly ignores header ordering on PY3 and uses record.values(), tying row
shape/order to mapping iteration order.

petl/test/test_method_shadow.py[211-217]
petl/io/avro.py[292-305]

Agent prompt

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

### Issue description
`test_fromavro_skip()` passes a list of plain dicts to `_read_rows_from()` and asserts a specific tuple shape/order, but the production code path for PY3 uses `tuple(record.values())`, which is driven by mapping iteration order.

### Issue Context
Even though CPython 3.7+ guarantees insertion order for `dict`, using an explicitly ordered mapping in the test makes the fixture deterministic and clearly documents the ordering contract the test expects.

### Fix Focus Areas
- petl/test/test_method_shadow.py[211-217]

### What to change
- Build `records` as `collections.OrderedDict([...])` (or another explicitly ordered mapping) in `test_fromavro_skip()`.
- Alternatively (broader change), update `AvroView._map_row_from()` to use `header` ordering on PY3 too (e.g., `tuple(record.get(col) for col in header)` when `header` is provided).

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[qodo-free-for-open-source-projects]

1. Test scan traverses tests 🐞 Bug ☼ Reliability

test_no_shadowed_api_names() attempts to skip the petl/test directory, but because it doesn’t prune
os.walk() traversal it still parses petl/test/* subdirectories. This makes the scan depend on
test-only modules and can break or misbehave under non-UTF8 default encodings when reading test
sources containing non-ASCII characters.

Agent Prompt

### Issue description
`test_no_shadowed_api_names()` uses `os.walk()` and `continue` to skip a directory named `test`, but this does not prevent `os.walk()` from descending into `petl/test/...`. As a result, the scan parses test modules too, and reads them with `open(path)` using the platform default encoding.

### Issue Context
Some `petl/test/...` files contain non-ASCII source (e.g., `u"é"`). Reading them via `open(path)` without honoring the source encoding cookie makes the test less portable and can fail under ASCII (or otherwise incompatible) locales.

### Fix Focus Areas
- petl/test/test_method_shadow.py[314-328]

### What to change
- Prune traversal by capturing `dirnames` from `os.walk()` and removing `test` (and potentially `__pycache__`) in-place so subdirectories under `petl/test` aren’t visited.
- Use an encoding-aware opener for Python source (e.g., `tokenize.open` on Py3, or a small helper that respects `# -*- coding: ... -*-`) instead of `open(path)`.

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

[qodo-free-for-open-source-projects]

2. Dict-order dependent test 🐞 Bug ☼ Reliability

test_fromavro_skip() constructs plain dict records but AvroView._map_row_from() uses
tuple(record.values()) on Python 3, so output column order follows mapping iteration order rather
than the provided header. This makes the new test depend on dict-order guarantees (or implementation
details) instead of explicitly modeling schema/header ordering.

Agent Prompt

### Issue description
`test_fromavro_skip()` passes a list of plain dicts to `_read_rows_from()` and asserts a specific tuple shape/order, but the production code path for PY3 uses `tuple(record.values())`, which is driven by mapping iteration order.

### Issue Context
Even though CPython 3.7+ guarantees insertion order for `dict`, using an explicitly ordered mapping in the test makes the fixture deterministic and clearly documents the ordering contract the test expects.

### Fix Focus Areas
- petl/test/test_method_shadow.py[211-217]

### What to change
- Build `records` as `collections.OrderedDict([...])` (or another explicitly ordered mapping) in `test_fromavro_skip()`.
- Alternatively (broader change), update `AvroView._map_row_from()` to use `header` ordering on PY3 too (e.g., `tuple(record.get(col) for col in header)` when `header` is provided).

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

[gaoflow]

Both review findings are addressed in the head commit:

• The scan now prunes test out of the os.walk dirnames instead of only checking the current directory's basename, and reads sources as bytes so the parser applies each module's own coding declaration. The old version was in fact failing all nine Windows jobs on cp1252 for exactly that reason.
• The avro records are OrderedDict now, so the expected row order does not lean on dict iteration order.

29/29 checks green.