Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grass.temporal: fix regression in printing metadata #4328

Merged
merged 2 commits into from
Sep 22, 2024

Conversation

petrasovaa
Copy link
Contributor

Fixes #4182 (t.info fails for single registered raster). I tried to work with the current design, although it might have been just easier to revert 7df1a7e, the method inheritance doesn't work very well there.

Besides the reported error, STDSMetadataBase.print_info() was also failing. I tested it with the currently deactivated doctests (there are issue running the doctests unrelated to this).

@petrasovaa petrasovaa added bug Something isn't working backport to 8.4 PR needs to be backported to release branch 8.4 labels Sep 16, 2024
@petrasovaa petrasovaa added this to the 8.4.1 milestone Sep 16, 2024
@github-actions github-actions bot added Python Related code is in Python libraries labels Sep 16, 2024
ninsbl
ninsbl previously approved these changes Sep 19, 2024
Copy link
Member

@ninsbl ninsbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general.

The one comment is probably a leftover and can be removed?

You may consider using just one print statement and multile f-strings in the print function. That would be minimally faster and less code, but that is not essential, I guess...

@petrasovaa
Copy link
Contributor Author

Looks good in general.

The one comment is probably a leftover and can be removed?

You may consider using just one print statement and multile f-strings in the print function. That would be minimally faster and less code, but that is not essential, I guess...

Right, but probably out of scope of this PR, there are many places in there that would have to be changed.

Copy link
Contributor

@veroandreo veroandreo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected now! Thanks!

@veroandreo veroandreo merged commit 6d046ec into OSGeo:main Sep 22, 2024
26 checks passed
@echoix
Copy link
Member

echoix commented Sep 22, 2024

Before merging, can we rerun with main merged here, as tests on Windows are more strict now

@veroandreo
Copy link
Contributor

Before merging, can we rerun with main merged here, as tests on Windows are more strict now

Seems I clicked merge while you were typing this? ops... It was all green ;-)

@echoix
Copy link
Member

echoix commented Sep 22, 2024

Ya, once I saw the email I looked at it and was writing. It was all green from 60 commits behind main. We'll see how it goes, maybe it'll need a follow-up or not. We'll have our answer in an hour or so

@echoix
Copy link
Member

echoix commented Sep 22, 2024

So yes, there are some unexpected successes, as some new tests are passing. If the affected tests were solved by this PR (not an unintended side-effect that we would want to be signalled), we need to remove the xfail_windows decorator on these tests.

@wenzeslaus
Copy link
Member

...It was all green...

The choice is between requiring the updated branch before merging (resulting in more CI wait time) or need to judge case-by-case if update is needed with occasional failed runs on main (current state).

@neteler
Copy link
Member

neteler commented Sep 23, 2024

Can I backport this or anything needed first in G84?

@petrasovaa petrasovaa deleted the fix-temporal-metadata-print branch September 24, 2024 02:23
petrasovaa added a commit that referenced this pull request Sep 24, 2024
* grass.temporal: fix regression in printing metadata

* Update python/grass/temporal/metadata.py

Co-authored-by: Stefan Blumentrath <[email protected]>

---------

Co-authored-by: Stefan Blumentrath <[email protected]>
@petrasovaa petrasovaa removed the backport to 8.4 PR needs to be backported to release branch 8.4 label Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libraries Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] t.info fails for single registered raster
6 participants