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

Various read write fixes #257

Merged
merged 11 commits into from
Nov 20, 2023
Merged

Various read write fixes #257

merged 11 commits into from
Nov 20, 2023

Conversation

HLWeil
Copy link
Member

@HLWeil HLWeil commented Nov 19, 2023

  • Reading ISA from Contracts will now not return Assay and Study objects, when they are only registered in the investigation file, but have no corresponding folder/file in the filesystem.
  • added a ARC.setFileSystem method, [BUG] Deleting assay from ARC will produce faulty write contracts #250
  • investigation spreadsheet writer previously failed when it contained a study that was only registered, but no corresponding study object
  • reworked test objects for full arc read-in

Copy link
Collaborator

@Freymaurer Freymaurer left a comment

Choose a reason for hiding this comment

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

I want to add ARC level api for removing assay/study, which will return a real REMOVE contract and handle fs updating internal. Let me know what you think about it.

@@ -194,7 +194,8 @@ module ArcInvestigation =
yield SparseRow.fromValues [contactsLabel]
yield! Contacts.toRows (Some contactsLabelPrefix) (List.ofArray investigation.Contacts)

for study in investigation.RegisteredStudies do
for studyIdentifier in investigation.RegisteredStudyIdentifiers do
Copy link
Collaborator

Choose a reason for hiding this comment

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

What exactly is happening here? Can you add a few comments?

@@ -19,7 +19,7 @@ let [<Literal>] ValidStudyFileNamePattern = @"^(studies(\/|\\))?(?<identifier>"
let checkValidCharacters (identifier: string) =
let regex = new Regex(ValidIdentifierPattern)
let isValid = regex.IsMatch(identifier)
if not isValid then failwith "New identifier contains forbidden characters! Allowed characters are: letters, digits, underscore (_), dash (-) and whitespace ( )."
if not isValid then failwith $"New identifier \"{identifier}\"contains forbidden characters! Allowed characters are: letters, digits, underscore (_), dash (-) and whitespace ( )."
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -71,6 +71,25 @@ type ARC(?isa : ISA.ArcInvestigation, ?cwl : CWL.CWL, ?fs : FileSystem.FileSyste

member this.FileSystem
with get() = _fs
and set(fs) = _fs <- fs

member this.RemoveAssay(assayIdentifier: string) =
Copy link
Member Author

Choose a reason for hiding this comment

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

These functions should contain update contracts for all registering objects (study,investigation)

Additional helpers:

  • ContractCreator Type extensions for ArcInvestigation, ArcStudy and ArcAssay in Contract namespace

@Freymaurer Freymaurer self-requested a review November 20, 2023 14:14
Copy link
Collaborator

@Freymaurer Freymaurer left a comment

Choose a reason for hiding this comment

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

lgtm if tests pass 😮

@Freymaurer Freymaurer merged commit a1fb10f into main Nov 20, 2023
2 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