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

STYLE: Move three internal helper functions into the unnamed namespace #72

Conversation

N-Dekker
Copy link
Collaborator

@N-Dekker N-Dekker commented Sep 3, 2024

Moved writeJson, jsonRead, and addCoordinateTransformations into the unnamed namespace of "itkOMEZarrNGFFImageIO.cxx".

Following C++ Core Guidelines, May 11, 2024, "Use an unnamed (anonymous) namespace for all internal/non-exported entities", http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf22-use-an-unnamed-anonymous-namespace-for-all-internalnon-exported-entities

@N-Dekker N-Dekker marked this pull request as ready for review September 3, 2024 19:12
@N-Dekker
Copy link
Collaborator Author

N-Dekker commented Sep 4, 2024

@dzenanz Some CI failures from build-macos-arm-py (10) from https://github.com/InsightSoftwareConsortium/ITKIOOMEZarrNGFF/actions/runs/10683486429/job/29611671206?pr=72#step:8:198

Fetching Python frameworks
./macpython-download-cache-and-build-module-wheels.sh: line 81: /Users/svc-dashboard/D/P/ITKPythonPackage/scripts/macpython-install-python.sh: No such file or directory
Building module wheels
./macpython-download-cache-and-build-module-wheels.sh: line 85: /Users/svc-dashboard/D/P/ITKPythonPackage/scripts/macpython-build-module-wheels.sh: No such file or directory

Do you have a clue? At least I think they are unrelated to this pull request 😇

@dzenanz
Copy link
Member

dzenanz commented Sep 4, 2024

https://github.com/InsightSoftwareConsortium/ITKMontage suffers from the same error, I don't know what broke, or how. What are the chances for you to get to it this week?

@dzenanz
Copy link
Member

dzenanz commented Sep 4, 2024

I might get to it next week, unless someone else gets to it before then.

@dzenanz
Copy link
Member

dzenanz commented Sep 4, 2024

Ahh, someone else is already on it: InsightSoftwareConsortium/ITKPythonPackage#283
What a pleasure 😄

@N-Dekker
Copy link
Collaborator Author

N-Dekker commented Sep 6, 2024

@dzenanz Can you please rerun the CI for build-macos-arm-py, for this pull request? Or otherwise just merge, and pray that it's okay 😺

@dzenanz
Copy link
Member

dzenanz commented Sep 6, 2024

The fix is ready. Let me apply it to this repo too.

@dzenanz
Copy link
Member

dzenanz commented Sep 6, 2024

I am hoping this should be enough to fix it: #73.

@dzenanz
Copy link
Member

dzenanz commented Sep 6, 2024

Rebasing on main should fix CI.

Moved `writeJson`, `jsonRead`, and `addCoordinateTransformations` into the
unnamed namespace of "itkOMEZarrNGFFImageIO.cxx".

Following C++ Core Guidelines, May 11, 2024, "Use an unnamed (anonymous)
namespace for all internal/non-exported entities",
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf22-use-an-unnamed-anonymous-namespace-for-all-internalnon-exported-entities
@N-Dekker N-Dekker force-pushed the Move-three-functions-into-unnamed-namespace branch from 750d922 to 9bb60da Compare September 6, 2024 19:28
@N-Dekker
Copy link
Collaborator Author

N-Dekker commented Sep 6, 2024

Now Build, test, package / build-windows-python-packages (8) fails, saying:


Invoke-WebRequest: D:\a\im\windows-download-cache-and-build-module-wheels.ps1:64
Line |
  64 |    Invoke-WebRequest -Uri "https://github.com/InsightSoftwareConsortiu …
     |    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | One or more errors occurred. (The response ended prematurely. (ResponseEnded))
Error: Process completed with exit code 1.

At https://github.com/InsightSoftwareConsortium/ITKIOOMEZarrNGFF/actions/runs/10744126899/job/29800364622?pr=72#step:8:86

Any clue?

@dzenanz
Copy link
Member

dzenanz commented Sep 6, 2024

That is most likely some network hiccup. We can re-run failed tests one the others finish running.

@dzenanz dzenanz merged commit 4e22d04 into InsightSoftwareConsortium:main Sep 9, 2024
18 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.

2 participants