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

Blob Refactor #4348

Merged
merged 18 commits into from
Mar 9, 2021
Merged

Blob Refactor #4348

merged 18 commits into from
Mar 9, 2021

Conversation

jessemapel
Copy link
Contributor

Description

This PR completes RFC 6, the Blob refactor so that Classes uses Blobs to serialize themselves, but don't inherit from Blob. This is API breaking and several classes were removed because their functionality independent of Blob was too small. This also adds new gtests for several of the Blob classes.

There are several failing app tests, but they are due to slightly different sizes in the Blobs because of how the storage method was cleaned up. Updated test data is ready to be checked in one this is merged.

Related Issue

#4082

Motivation and Context

This makes it much easier to serial new objects to a cube because they do not need to inherit an entire new class with members, particularly for classes that already have a parent class. This also reduces the code base by a good amount and reduces the amount of functionality that must be tested on many classes.

How Has This Been Tested?

All Blob classes were tested by hand and with automated tests. In particular, the CubeStretch/StretchBlob changes were hand tested and verified.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation change (update to the documentation; no code change)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read and agree to abide by the Code of Conduct
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have added myself to the .zenodo.json document.
  • I have added any user impacting changes to the CHANGELOG.md document.

Licensing

This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words:

This work is free and unencumbered software released into the public domain. In jurisdictions that recognize copyright laws, the author or authors of this software dedicate any and all copyright interest in the software to the public domain.

  • I dedicate any and all copyright interest in this software to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this software under copyright law.

acpaquette and others added 16 commits February 22, 2021 11:00
* Converted Imagepolygon to no longer be a subclass of blob

* Updated Imagepolygon with constructor chaining

* Fixed error message
* Initial refactor of OriginalLabel class

* Addressed PR feedback

* Fixed error message

Co-authored-by: Jesse Mapel <jmapel@usgs.gov>
* Ported Table test to gtest

* Table refactor

* Fixed merge issue

* Fixed merge issues
* Initial refactor of OriginalLabel class

* Addressed PR feedback

* Fixed error message

* Remove blob inheritance from originalxmllabel

* Removed commented / old code

* Added byte order specification
* Adds History object tests

* Updated blob tests to use a non-pointer variable
* Removed blob inheritance from stretchblob

* Addressed feedback + working read/write

* Updated cubestretch with the stretch blob functionality

* Removed stretchBlob and updated cubestretch/cube with read write functionality

* Replaced readStretchBlob with readCubeStretch

* Fixed segfault

* Removed debug print statements

* Added check for existence of OriginalLabel before attempting to propagate

* Addressed PR feedback

Co-authored-by: Adam Paquette <acpaquette@usgs.gov>
* Converted StringBlob

* Removed StretchBlob class

* Changed Blob setData to copy

* Added csminit change and docs

* Removed old memcopy
* Implemented old history functionality

* Removed old history unit test

* Fixed original label reading
* Fixed test failures

* More clean-up

* Fixed marci2isis test
* Redid Blob writing

* Added docs
AustinSanders
AustinSanders previously approved these changes Mar 8, 2021
Copy link
Contributor

@AustinSanders AustinSanders left a comment

Choose a reason for hiding this comment

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

Other than history comments, I didn't see anything that really needs to be changed.

isis/src/qisis/apps/qtie/QtieTool.cpp Outdated Show resolved Hide resolved
acpaquette
acpaquette previously approved these changes Mar 8, 2021
Copy link
Collaborator

@acpaquette acpaquette left a comment

Choose a reason for hiding this comment

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

Overall looks good! There is one failing unit test, isis_unit_test_GisTopology. It's something that I think I or Jesse could take care of

isis/src/qisis/apps/qtie/QtieTool.cpp Outdated Show resolved Hide resolved
isis/src/base/apps/csminit/csminit.cpp Outdated Show resolved Hide resolved
isis/src/base/objs/Blob/Blob.cpp Show resolved Hide resolved
isis/src/base/objs/OriginalXmlLabel/OriginalXmlLabel.cpp Outdated Show resolved Hide resolved
isis/src/qisis/apps/qtie/QtieTool.cpp Outdated Show resolved Hide resolved
isis/src/qisis/apps/qtie/QtieTool.cpp Outdated Show resolved Hide resolved
@jessemapel jessemapel dismissed stale reviews from acpaquette and AustinSanders via 67e8c9b March 8, 2021 21:37
@jessemapel
Copy link
Contributor Author

CI is good, the new app test failures are all expected and we have test data ready to check in. @acpaquette @AustinSanders just need a re-approval on this to merge.

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