-
Notifications
You must be signed in to change notification settings - Fork 8
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
Rework data nodes #356
Rework data nodes #356
Conversation
…headers with "Data" #93
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you write tests to check if backwards compatibility is maintained?
src/Core/Conversion.fs
Outdated
| CompositeHeader.Output IOType.DerivedDataFile -> ProcessOutput.createDerivedData(value.ToString()) | ||
| CompositeHeader.Output IOType.Data -> ProcessOutput.createImageFile(value.ToString()) | ||
| CompositeHeader.Output IOType.Data -> ProcessOutput.createRawData(value.ToString()) | ||
| CompositeHeader.Output IOType.Data -> ProcessOutput.createDerivedData(value.ToString()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/Core/ArcTypes.fs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
datamap missing in ToString override
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/Core/Process/Data.fs
Outdated
@@ -1,31 +1,36 @@ | |||
namespace ARCtrl.Process | |||
namespace ARCtrl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not ARCtrl.Process
anymore? but still located in Process
folder? Depending on the answer, the record type should be changed to class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the type is now also used in the ArcTable. But as it is now being referenced both from ArcTable and Process, finding a place in the project is a bit difficult.
@@ -15,10 +15,12 @@ type CompositeCell = | |||
/// | |||
/// https://isa-specs.readthedocs.io/en/latest/isatab.html#unit | |||
| Unitized of string*OntologyAnnotation | |||
| Data of Data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this Data is the Data from Process/Data.fs then Data
must be changed to class with AttachMembers!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do!
tests/Core/ARCtrl.Core.Tests.fsproj
Outdated
<PackageReference Include="Fable.Pyxpecto" Version="1.0.1" /> | ||
<PackageReference Include="Fable.Core" Version="4.2.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fable.Core dependency seems irrelevant as this should come from project reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ARC-Specification#93
#300
ARC-Specification#93
ARC-Specification#104