Skip to content

Move data manipulation functions to external header #94

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

Merged

Conversation

ebald19
Copy link
Contributor

@ebald19 ebald19 commented May 27, 2025

Issue

There were multiple functions within the ScancoIO class that did not depend on or use any class-specific values. These functions are all related to manipulating raw data or strings and decoding/conversions to and from typed data. These currently clutter among the related IO functions for the class.

Additionally, these functions affect part of the issues seen in this issue. Specifically, the dates when written to an ISQ header are not correctly converted to the OpenVMS timestamps which have a different epoch. This caused 'garbage' dates to be written with the buggy conversion.

Proposed Changes

  • Move all data manipulation functions not related to the ScancoIO class out of the class
  • Update moved function documentation to match ITK and Doxygen requirements
  • Fix the EncodeDate function to be EncodeCurrentDate
  • Add EncodeDateFromString to encode a given date string to the VMS format

Notes:

  • There is no change to the external API here, only internal organization
  • itkDataManipulation.h/cxx are added files and an associated line is added for these to be compiled with cmake in the CMakeLists.txt
    -> This file's name and organization method can be modified easily if preferred

Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me. Someone else should check this too.

Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Could you rebase on top of main branch, so we get the mostly-working CI?

@dzenanz
Copy link
Member

dzenanz commented May 28, 2025

You can see errors and warnings here. Search for ITKIOScanco- in Build Name column, then click on highlighted red or yellow number. Here is one example build error page.

Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

These numerous commits should be squashed down to just a few, or maybe even just one.

@ebald19 ebald19 force-pushed the bugfix/fix_isq_header_write branch 2 times, most recently from a216805 to 3864354 Compare May 29, 2025 19:01
@@ -1,119 +0,0 @@
/*=========================================================================
Copy link
Member

Choose a reason for hiding this comment

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

I assume these file deletions should have been in a previous commit? Also it might make sense to squash commits 2 and 3, to avoid appearance of itkDataManipulation.h only to be immediately renamed.

hjmjohnson and others added 2 commits June 2, 2025 09:13
STYLE: Move unrelated data manipulation functions out of main class

STYLE: Add and update doxygen comments for functions

BUG: remove depreciated sprintf
@ebald19 ebald19 force-pushed the bugfix/fix_isq_header_write branch from 3864354 to 6da674c Compare June 2, 2025 15:15
@dzenanz
Copy link
Member

dzenanz commented Jun 2, 2025

If you mark this as "ready for review" I will squash-merge it.

@ebald19 ebald19 marked this pull request as ready for review June 2, 2025 19:03
@dzenanz dzenanz merged commit 434b550 into InsightSoftwareConsortium:main Jun 3, 2025
23 checks passed
@ebald19 ebald19 deleted the bugfix/fix_isq_header_write branch June 25, 2025 16:15
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.

3 participants