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

Fix GrepDateTest #30

Conversation

adamjtaylor
Copy link
Collaborator

Amends GrepDateTest to

  • Negate the exit code such that a match returns exit 1 (test fails) and no match returns exit 0 (test passes)
  • Removes -w option so that terms such as <AcquisitionDate> where the risk term is not a isolated word now match
  • Adds -a option to explicitly treat binary files as text
  • Adds -q option to suppress output as is not needed by the test

Note that for TIFF files this only searches within values of Tags where the content is a string, such as ImageDescription
We may wish to add an additional test that searches the DateTime tag itself.

@adamjtaylor
Copy link
Collaborator Author

FYI @thomasyu888 - looks like I can't explicitly assign you as a reviewer.

@adamjtaylor
Copy link
Collaborator Author

Tests failing as this is a branch on my fork.
@thomasyu888 could I have my permissions in this repo elevated so that I can create branches in the repo and assign reviewers.

@adamjtaylor
Copy link
Collaborator Author

adamjtaylor commented Apr 13, 2023

As this test is now part of the TiffSuite, I am tempted to extend it here to grep within the output of tifftools dump

eg
! tifftools dump image.tiff | grep -Eiaq 'date|time'

This would allow for the names of tags such as DateTime to be matched within the same test rather than having to run a separate test.

The alternative is to add a new test, TiffDateTimeTagTest. I would probablly still use a grep on tifftools dump to do this to ensure all IFDs and subIFDs are looked at.

eg.
! tifftools dump image.tif | grep -aq 'DateTime 306 (0x132) ASCII'

Both would require switching to a container containing tifftools

@thomasyu888 thomasyu888 self-requested a review April 13, 2023 21:55
@thomasyu888
Copy link
Contributor

Thanks @adamjtaylor I'm happy to give you write permission to the repo. One second while I make some adjustments. I like what you've proposed for a test. We can certainly create a new test OR just repurpose this GrepDateTest to be TiffDateTimeTagTest

@thomasyu888 thomasyu888 changed the base branch from main to ataylor/fix-grep-date-test April 13, 2023 21:59
@thomasyu888 thomasyu888 merged commit 1629d85 into Sage-Bionetworks-Workflows:ataylor/fix-grep-date-test Apr 13, 2023
@thomasyu888 thomasyu888 mentioned this pull request Apr 13, 2023
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