[bhuvan-somisetty]

Description

This PR resolves issues in grass.pygrass.vector.basic.Ilist:

1. Fixes attribute access in Ilist.extend() and Ilist.remove(): changes ilist.ilist and value.ilist to ilist.c_ilist and value.c_ilist to pass the correct ctypes pointer (struct ilist *) to libvect.Vect_list_append_list and libvect.Vect_list_delete_list.
2. Fixes Ilist.append() to raise GrassError when libvect.Vect_list_append returns non-zero (failure) rather than executing a bare raise.
3. Cleans up redundant inline imports in basic.py.
4. Adds unit tests for Ilist in python/grass/pygrass/vector/testsuite/test_basic.py.

Motivation and context

Ilist stores its underlying ctypes pointer attribute as self.c_ilist. Calling Ilist.extend() or Ilist.remove() with an Ilist instance previously raised AttributeError: 'Ilist' object has no attribute 'ilist'. Furthermore, calling Ilist.append() previously raised RuntimeError due to a bare raise when the C function returned an error code.

Fixes #7976.

How has this been tested?

Added unit tests in python/grass/pygrass/vector/testsuite/test_basic.py covering:

• Ilist empty and list-based initialization
• Indexing and slicing
• __setitem__ and duplicate handling
• contains and __contains__ (in operator)
• append() normal and error handling with GrassError
• extend() with Python lists and Ilist instances
• remove() with integers, Ilist instances, and iterables
• reset()

Screenshots (if appropriate)

N/A

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as before)

Checklist

• PR title provides summary of the changes and starts with one of the pre-defined prefixes
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.

[echoix]

Also, can you check if the previous intention was to have a python-side copy of c_I lost to not directly play with that?

[bhuvan-somisetty]

Checked the design: Ilist (along with Bbox, BoxList, Cats) is structured as a direct ctypes wrapper around the C struct ilist (self.c_ilist). All operations (__len__, __iter__, __getitem__, __setitem__, append, remove) operate directly on self.c_ilist rather than keeping a Python-side copy.

[echoix]

I ran that through only Sonnet 5.5 (not a very strong review), but it had some good ideas, and also thinks the same as me on another point.

Briefly, test_remove_invalid should be adjusted, the test might be better suited as pytest (unless you had a strong reason to keep it gunittest).

See the full thread below:

Details

Review of #7977: grass.pygrass: Fix Ilist.extend and Ilist.remove accessing non-existent attribute

I read the diff and checked it against lib/vector/Vlib/list.c and pygrass/errors.py. I didn't run the tests. I haven't posted anything on GitHub.

Verdict: the fix is correct and I'd approve it. Only the first of the three requested changes below matters.

What I checked

• extend and remove: ilist.ilist and value.ilist never existed, because the attribute is c_ilist. The change to .c_ilist is right, and it fixes Ilist([..]) construction from another Ilist too.
• append: the bare raise was a bug, since it raised RuntimeError outside an except block. The C docs say Vect_list_append returns 0 on success and 1 on error. It also returns 0 when the value is already in the list, so raising only on non-zero is correct and won't break duplicate appends.
• GrassError import: pygrass.errors just re-exports grass.exceptions.GrassError, so it is the same class. Importing it from grass.exceptions at module level is fine, and it avoids the inline import. The import reordering is minor scope creep but harmless.

Requested changes

1. test_remove_invalid passes for the wrong reason.
   • "invalid" is a str, which is Iterable, so it takes the iterable branch.
   • int("i") then raises ValueError. The else: raise ValueError("... not supported") branch is never reached.
   • Use a non-iterable such as ilist.remove(1.5) or None, so the test covers the branch its docstring describes.
2. Test location and style. AGENTS.md and doc/development/testing.md say new tests should be pytest in tests/. testsuite/ with gunittest is the legacy layout. Ilist wraps a plain ctypes struct and needs no GRASS session, so a simple pytest file would be easier to run. I'd ask for a pytest version, or confirm that maintainers are fine with gunittest here.
3. Tests for the return codes (optional). Vect_list_append_list and Vect_list_delete_list return 1 on NULL, and extend and remove ignore that. The pointers can't be NULL here, so this isn't worth acting on.

Nits

• test_append_error patches libvect.Vect_list_append to return 1. That is acceptable, since a real failure can't be provoked, but a short comment saying why would help.
• The PR description says "Fixes #7976". That's fine, as long as the PR body keeps the template headings.

[bhuvan-somisetty]

Thanks for the review @echoix!

I've addressed the feedback:

• Converted the test suite to pytest in python/grass/pygrass/vector/tests/test_basic.py and removed the legacy gunittest file.
• Updated test_remove_invalid with non-iterables (1.5, None) so it directly tests the unsupported type branch.
• Added an explanatory comment in test_append_error for the Vect_list_append mock.
• Rebased on main.

[echoix]

Pick what's still appropriate to improve here vs ignoring or in another PR, I don't feel strong about any of them, tests are better than no tests, and some of the findings are not concerned by the goal of your edit.

AI review

Code review of #7977

Seven findings, listed in the order the review returned them. The review did not assign severities.

#	Location	Finding
1	python/grass/pygrass/vector/basic.py:337	Ilist.append() passes the raw value to the ctypes Vect_list_append with no int() coercion.
2	basic.py:362	Ilist.remove() ignores the return codes of Vect_list_delete and Vect_list_delete_list.
3	basic.py:362	Ilist.remove() dispatches on isinstance(value, int), so bool and numpy integers behave inconsistently.
4	basic.py:337	The new GrassError in append is effectively unreachable.
5	basic.py:549	GrassError is imported from grass.exceptions, while sibling pygrass modules may use grass.pygrass.errors.
6	tests/test_basic.py:1	The tests have no GRASS session fixture and miss negative-index paths.
7	basic.py:328	Ilist.__contains__ iterates in Python instead of calling the existing contains().

Details

1. append has no type coercion.

• Ilist().append(1.5) or Ilist().append('3') raises ctypes.ArgumentError, so the new GrassError never fires.
• __setitem__ calls int(value), so the two methods raise different errors for the same bad input.
• The new tests do not cover this.

2. remove ignores return codes.

• Vect_list_delete returns 1 when the list is NULL, so that failure is silent.
• Removing an absent value is a silent no-op, whereas list.remove raises ValueError.
• The PR fixes the error path in append but not in remove, and the new test_remove_* tests check neither case.

3. remove type dispatch.

• remove(numpy.int64(2)) is not an int or an Ilist, and numpy scalars are not Iterable.
• It falls through to ValueError('not supported').
• test_remove_invalid asserts this behaviour, which locks it in.

4. Unreachable GrassError.

• Vect_list_append only returns 1 for a NULL list, and G_realloc aborts the process on out-of-memory.
• test_append_error can only pass by mocking libvect.Vect_list_append, so it exercises a branch real code cannot reach and adds little coverage.

5. Import convention.

• Both import paths resolve to the same class, so nothing breaks.
• The PR still introduces a second import convention within pygrass.
• Separately, the % num_errors string in from_string is not translatable. The PR left it unchanged.

6. Test setup and coverage.

• The tests import grass.lib.vector at collection time without a session fixture. In an environment without the GRASS library paths, that gives collection errors instead of skips.
• test_getitem does not cover ilist[-100] or other out-of-range negative indices.
• A negative key below -n gives a negative index and reads value[-k] out of bounds without raising IndexError. This is an existing bug that the new test misses.

7. Duplicate membership check.

• __contains__ iterates with item in self.__iter__(), which is O(n) in Python.
• The existing contains() does the same check in one C call through Vect_val_in_list.
• The two paths could diverge. The PR edits the neighbouring methods but leaves this duplication.

[bhuvan-somisetty]

Thanks for the detailed review! I've updated the PR with the following fixes:

• Coerced the input value to int in append() to match __setitem__.
• Switched the GrassError import to grass.pygrass.errors.
• Fixed the negative index bounds check in __getitem__ so keys below -len raise IndexError (and added a test for it).
• Delegated __contains__ directly to self.contains().
• Added test coverage for float coercion in append().

[echoix]

Thanks for reworking a couple of times, looks fine with me. Waiting for CI to finish to see test results.

[bhuvan-somisetty]

@echoix PTAL