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

Add PRIDE validation package #16

Merged
merged 9 commits into from
Mar 11, 2024
Merged

Add PRIDE validation package #16

merged 9 commits into from
Mar 11, 2024

Conversation

omaus
Copy link
Collaborator

@omaus omaus commented Feb 21, 2024

This PR

@omaus omaus marked this pull request as ready for review February 22, 2024 17:07
@omaus omaus requested a review from kMutagene February 22, 2024 17:07
Copy link
Member

@kMutagene kMutagene left a comment

Choose a reason for hiding this comment

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

@omaus

Nice progress so far. I think my review comments on nfdi4plants/arc-validate#88 should be addressed before publishing (meanining: setting Publish true in the header).

Small request: could you add more metadata to the header? I know these are optional but especially tags would be nice. After that we can merge this so that it is accessible for the github api-based package installation already, and move to production once the helpers are refactored.

@kMutagene
Copy link
Member

@omaus note that publishing and merging into this repo are two separate things.

We can absolutely merge this when more metadata is added.

@kMutagene
Copy link
Member

@omaus sorry to nudge you so much but can you please add author and tag metadata so we can merge this? Should not take long.

@omaus
Copy link
Collaborator Author

omaus commented Feb 27, 2024

@omaus sorry to nudge you so much but can you please add author and tag metadata so we can merge this? Should not take long.

Yeah thx, pls check if it is done right (I'm esp. unsure about the publish: true). 👍🏻

@omaus omaus requested a review from kMutagene February 27, 2024 09:00
@kMutagene
Copy link
Member

kMutagene commented Feb 27, 2024

Lgtm, but pls do not set publish - this is intended for full immutable release, and iirc we want to adapt arcexpect api before that

@kMutagene
Copy link
Member

kMutagene commented Feb 27, 2024

But commiting to this repo makes the package available as prerease already

@omaus
Copy link
Collaborator Author

omaus commented Mar 7, 2024

Lgtm, but pls do not set publish - this is intended for full immutable release, and iirc we want to adapt arcexpect api before that

Done.

@kMutagene
Copy link
Member

kMutagene commented Mar 7, 2024

thanks @omaus, heads up that i will push some changes to your branch to test some ci capabilities

Copy link
Member

@kMutagene kMutagene left a comment

Choose a reason for hiding this comment

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

allright, testing pipeline seems to work and also caught multiple problems (see pipeline output).

Please make sure that the version in the yaml metadata is the same as indicated in the file name, and make sure that the script can run. Compilation of the script in ci fails, and i suspect that that is due to the versions of the nuget packages not being pinned, and ARCExpect API having changed since you submitted the PR.

I will add a test that checks that all nuget references are pinned.

@omaus
Copy link
Collaborator Author

omaus commented Mar 8, 2024

@kMutagene

Unit test says that version is wrong. I don't see why that should be the case. The file's name is "pride@1.0.0.fsx", YAML tags:

MajorVersion: 1
MinorVersion: 0
PatchVersion: 0

???

@kMutagene
Copy link
Member

???

Those are the default values the metadata class gets initialized with if the frontmatter is invalid !!!
Note that also the StagingDirectory.FileContent.All files have valid metadata test fails. The issue is that your file gets checked out with LF on windows instead of CRLF. Not sure why but i really do not care, ill just replace line endings on parsing frontmatter.

@kMutagene
Copy link
Member

Also, name in ontology tags should be Name, which is my bad.

@kMutagene kMutagene merged commit 61cca6b into main Mar 11, 2024
6 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.

[Feature] Add validation/QC-package for PRIDE
2 participants