Skip to content

Fix: Adding Pace and PySHiELD unit tests back in to workflow#66

Merged
fmalatino merged 14 commits into
NOAA-GFDL:developfrom
fmalatino:workflow_fix
Jul 22, 2025
Merged

Fix: Adding Pace and PySHiELD unit tests back in to workflow#66
fmalatino merged 14 commits into
NOAA-GFDL:developfrom
fmalatino:workflow_fix

Conversation

@fmalatino

@fmalatino fmalatino commented Jul 15, 2025

Copy link
Copy Markdown
Contributor

Description
This PR reintroduces the Pace and PySHiELD unit tests to the workflow. It also removes setup.cfg in favor of specifying its contents within pyproject.toml

Fixes # (issue)
If this is a hotfix to a released version, please specify it

How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also note
any relevant details for your test configuration (e.g. compiler, OS). Include
enough information so someone can reproduce your tests.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • Targeted model if this changed was triggered by a model need/shortcoming

@fmalatino fmalatino changed the title Fix: Adding Pace and PySHiELD unit tests Fix: Adding Pace and PySHiELD unit tests Jul 17, 2025
@fmalatino fmalatino marked this pull request as ready for review July 17, 2025 15:18
@fmalatino fmalatino changed the title Fix: Adding Pace and PySHiELD unit tests Fix: Adding Pace and PySHiELD unit tests back in to workflow Jul 17, 2025
@fmalatino fmalatino changed the title Fix: Adding Pace and PySHiELD unit tests back in to workflow Fix: Adding Pace and PySHiELD unit tests back in to workflow Jul 17, 2025

@romanc romanc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good to see the test harness working again.

I think adding an exception for "F401" (no unused imports) in the flake8 config shouldn't be and we should find a way to only allow unused imports in __init__.py files (as previously). If that takes longer than expected, feel free to break out re-enabling the pace / pyshield tests (which are independent of the pyproject.toml changes.

Comment thread pyproject.toml Outdated
Comment thread pyproject.toml Outdated
Comment thread pyproject.toml Outdated

[tool.flake8]
exclude = ["docs"]
extend-ignore = ["W503", "E302", "E203", "F841", "F401"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This adds the exception for F401 (no unused imports) to all files (not just __init__.py files as before). I think it would be good if we pruned unused imports in all files other than __init__.py (which can have convenience imports/exports).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In the __init__.py you can do

import .moduleA
import .moduleB

__all__ = ["moduleA", "moduleB"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, this is why the F401 exception should be there for __init__.py files. I'm arguing it should only be there for __init__.py files (and not for any other python files).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

But if you do the __all__, which is the spec, I don't think you need the F401 at all (I could be wrong)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah okay, today I learned ... Good to know!

@fmalatino fmalatino requested a review from romanc July 17, 2025 17:26
bensonr
bensonr previously approved these changes Jul 17, 2025
romanc
romanc previously approved these changes Jul 18, 2025

@romanc romanc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

@fmalatino fmalatino dismissed stale reviews from romanc and bensonr via 5177a0c July 21, 2025 14:33
oelbert
oelbert previously approved these changes Jul 21, 2025

@oelbert oelbert left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

@romanc romanc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Some small things we learned from the NDSL repo recently.

Comment thread .pre-commit-config.yaml Outdated
Comment thread .pre-commit-config.yaml Outdated
Comment thread .pre-commit-config.yaml Outdated
Comment thread .pre-commit-config.yaml Outdated
Comment thread pyproject.toml Outdated
@romanc romanc self-requested a review July 21, 2025 15:22

@romanc romanc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If everybody agrees that we should merge now, we can also do the above in a follow-up.

romanc
romanc previously approved these changes Jul 21, 2025

@romanc romanc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Seems like I need to formally approve to make a follow-up possible, so here we go.

@fmalatino

Copy link
Copy Markdown
Contributor Author

Seems like I need to formally approve to make a follow-up possible, so here we go.

I will make these changes in this PR as I think they are also relevant to this one and dismiss stale reviews

romanc
romanc previously approved these changes Jul 21, 2025

@romanc romanc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice! 🚀

bensonr
bensonr previously approved these changes Jul 22, 2025

@romanc romanc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ship it! 🚢

@fmalatino fmalatino merged commit 9a491c0 into NOAA-GFDL:develop Jul 22, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci: move all configration from setup.cfg into pyproject.toml

5 participants