[saket0187]

This PR is a part of #7484

Also, a quick question: Why is the output when using methods like method=delta is same as the default one, while it is different in t.rast.list? Is this intentional?

[petrasovaa]

This PR is a part of #7484

Also, a quick question: Why is the output when using methods like method=delta is same as the default one, while it is different in t.rast.list? Is this intentional?

I didn't check, but that sounds like a bug. Could you investigate and create an issue for that if needed (and ideally fix:))

[Valyrian-Code]

Nice to see more pytest coverage for the temporal tools. One small thing: with grass.tools.Tools, a non-zero return code already raises CalledModuleError, so the assert result.returncode == 0 checks are redundant (the call would fail before reaching them) and can be dropped.

Minor: in the fixture, since names and max_values are the same length, zip(..., strict=True) would catch an accidental mismatch if either list changes later.

Looks good overall.

[echoix]

Nice to see more pytest coverage for the temporal tools. One small thing: with grass.tools.Tools, a non-zero return code already raises CalledModuleError, so the assert result.returncode == 0 checks are redundant (the call would fail before reaching them) and can be dropped.

It's redundant only if the tests for grass tools api have a test checking this situation to make sure this assumption is still valid in the future, and that there isn't weird workarounds specific to the temporal part used (that would skip the place where there is this assumption)

Minor: in the fixture, since names and max_values are the same length, zip(..., strict=True) would catch an accidental mismatch if either list changes later.

Agree, and our minimum supported python version is 3.10 (see the pyproject.toml for example), that argument is available.

There is a rule that can be used in ruff for that, B905 https://docs.astral.sh/ruff/rules/zip-without-explicit-strict/, and it shouldn't be inactive, I'm not sure why it wasn't caught. ah, strict=False was written

@Valyrian-Code That kind of small suggestion is easier to apply when adding a review comment with a specific suggestion. Even if a comment or requested changes from collaborators outside the repository aren't considered by GitHub when evaluating if the configured rulesets allow a PR to be merged, they still appear as a grey-version of the same icon, and can be useful for the member that got the review.

Looks good overall.

[Valyrian-Code]

Thanks @echoix, good points.

On the returncode assert: fair, that is more defensive than I implied. It is only safe to drop if grass.tools itself has a test pinning the raise-on-non-zero behaviour and nothing temporal-specific bypasses it. Without that guarantee, keeping the assert is reasonable.

On B905: I think it passed because the rule only requires strict= to be set explicitly, and strict=False is explicit, so it is compliant. It does not enforce True. So this is just a preference to use strict=True here, since the two lists are meant to be the same length.

And thanks for the tip on adding a review comment with a specific suggestion. That makes sense, I will use that for small changes going forward.

[echoix]

And thanks for the tip on adding a review comment with a specific suggestion. That makes sense, I will use that for small changes going forward.

Why not try it here now? You already gave the context in the long pr conversation, so just the suggestion is appropriate

[saket0187]

@petrasovaa @echoix If this looks good, I was thinking we can merge this so that I can create the format option PR. I will create a separate, dedicated PR for that method fix as well.

[echoix]

I had to update the branch because the windows build would have failed, and it was recently fixed on main.

[echoix]

This PR is a part of #7484

Also, a quick question: Why is the output when using methods like method=delta is same as the default one, while it is different in t.rast.list? Is this intentional?

I didn't check, but that sounds like a bug. Could you investigate and create an issue for that if needed (and ideally fix:))

Can this PR be merged, is this solved (or does it impact these tests)? Cc @petrasovaa

[echoix]

I don't have anything else to say