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

FileSystemTree additions and Payload detection #189

Merged
merged 4 commits into from
Sep 14, 2023
Merged

Conversation

kMutagene
Copy link
Member

closes #188:

  • Add FileSystemTree.FilterFolders
  • FileSystemTree.FilterFiles now only filters files (returns empty folders instead of removing them)
  • fix some static member bindings
  • Add tests for FilterFolders
  • Tests now test both instance and static methods

closes #187:

  • Add GetRegisteredPayload and GetAdditionalPayload
  • Add tests

- Add FileSystemTree.FilterFolders
- FileSystemTree.FilterFiles now only filters files (returns empty folders instead of removing them)
- fix some static member bindings
- Add tests for FilterFolders
- Tests now test both instance and static methods
-Add `GetRegisteredPayload` and `GetAdditionalPayload`

- Add tests
@kMutagene kMutagene changed the title Implement #187 FileSystemTree additions and Payload detection Sep 12, 2023
kMutagene added a commit to nfdi4plants/arc-export that referenced this pull request Sep 12, 2023
- iterate through all tables instead of the first only
- use ToFreeTextCell(), which is a proper conversion instead of AsFreeTextCell which is a match that can fail
yield $"{assayFoldername}/protocols/{kv.Value.AsFreeText}" // from assay root > protocols
for table in a.Tables do
for kv in table.Values do
let textValue = kv.Value.ToFreeTextCell().AsFreeText
Copy link
Member Author

Choose a reason for hiding this comment

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

@Freymaurer @HLWeil is this the way to access a cells text? If that's the case, I suggest we add a convenience function for this

Copy link
Member

Choose a reason for hiding this comment

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

Yup just tested it out. This definitely needs some convenience function.

Copy link
Member

@HLWeil HLWeil left a comment

Choose a reason for hiding this comment

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

Use the literals. Else everything is good (according to the current state of the repository) as far as I can see!

yield $"{assayFoldername}/protocols/{kv.Value.AsFreeText}" // from assay root > protocols
for table in a.Tables do
for kv in table.Values do
let textValue = kv.Value.ToFreeTextCell().AsFreeText
Copy link
Member

Choose a reason for hiding this comment

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

Yup just tested it out. This definitely needs some convenience function.

let studyFoldername = $"studies/{s.Identifier}"

set [
yield $"{studyFoldername}/isa.study.xlsx"
Copy link
Member

Choose a reason for hiding this comment

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

Sry I completely forgot to mention this yesterday. You can and should use the helper functions for this:

open ARCtrl.ISA

Identifier.Study.fileNameFromIdentifier "MyStudy"

->
val it: string = "studies/MyStudy/isa.study.xlsx"

Or at least (also considering there is no such helper function for the Readme), use the provided literals:

ARCtrl.Path.StudiesFolderName
ARCtrl.Path.StudyFileName
ARCtrl.Path.READMEFileName

Copy link
Member Author

Choose a reason for hiding this comment

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

done

/// <param name="IgnoreHidden">Wether or not to ignore hidden files and folders starting with '.'. If true, no hidden files are included in the result. (default: true)</param>
member this.GetRegisteredPayload(?IgnoreHidden:bool) =

let isaCopy = _isa |> Option.map (fun i -> i.Copy()) // not sure if needed, but let's be safe
Copy link
Member

Choose a reason for hiding this comment

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

As far as I see it, this is not needed. But we definitely need structural equality functions @Freymaurer. So functions like this here can be tested for doing no change to the ARC itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

then i guess it is better to leave it for now

@HLWeil HLWeil merged commit 4e676ea into developer Sep 14, 2023
2 checks passed
@HLWeil HLWeil deleted the implement-#187 branch September 20, 2023 13:07
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