-
Notifications
You must be signed in to change notification settings - Fork 23
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
rebrand nf-validation to nf-schema #1
Conversation
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.
Looks good but seems several things got mangled - mostly URLs.
Also URLs that point to commits/blob ids are the same. I think that is right but might be worth double-checking they exist!
Co-authored-by: awgymer <[email protected]>
Co-authored-by: awgymer <[email protected]>
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.
Just one little issue still I think
docs/images/nf-validation.svg
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.
Needs replacing and renaming I think?
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.
Yep, I'm not sure how to make this logo (@mirpedrol you made it, right?)
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.
Yes, I will push a new image with the new name. We should also think of a new logo later.
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.
done! The check mark doesn't make a lot of sense now, do you have better ideas?
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.
Maybe the icon without the check mark? It kinda looks like schemafile
docs/images/nf-validation.svg
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.
Yes, I will push a new image with the new name. We should also think of a new logo later.
plugins/nf-schema/src/main/nextflow/validation/JsonSchemaValidator.groovy
Outdated
Show resolved
Hide resolved
Co-authored-by: Júlia Mir Pedrol <[email protected]>
Co-authored-by: Júlia Mir Pedrol <[email protected]>
…ator.groovy Co-authored-by: Júlia Mir Pedrol <[email protected]>
.github/workflows/build-docs.yml
Outdated
@@ -19,7 +19,7 @@ jobs: | |||
python-version: 3.x | |||
- run: echo "cache_id=$(date --utc '+%V')" >> $GITHUB_ENV | |||
- name: Obtain version from MANIFEST.MF | |||
run: echo "plugin_version=$(grep "Plugin-Version" plugins/nf-validation/src/resources/META-INF/MANIFEST.MF | awk '{print $2}' | awk -F '.' '{print $1"."$2}')" >> $GITHUB_ENV | |||
run: echo "plugin_version=$(grep "Plugin-Version" plugins/nf-schema/resources/META-INF/MANIFEST.MF | awk '{print $2}' | awk -F '.' '{print $1"."$2}')" >> $GITHUB_ENV |
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.
no src/
?
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.
Good spot!!!
docs/images/nf-schema.svg
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.
the viewbox is too small, the a is out of it
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.
Weird, it's fine for me 😕
@@ -9,12 +9,12 @@ hide: | |||
You can see an example JSON Schema for a Nextflow pipeline `nextflow_schema.json` file below. | |||
|
|||
This file was generated from the nf-core pipeline template, using `nf-core create`. | |||
It is used as a test fixture in the nf-validation package [here](https://github.com/nextflow-io/nf-validation/blob/master/plugins/nf-validation/src/testResources/nextflow_schema.json). | |||
It is used as a test fixture in the nf-schema package [here](https://github.com/nextflow-io/nf-schema/master/plugins/nf-schema/resources/nextflow_schema.json). |
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.
sure about that url?
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.
Ah missed another one 😁 Thanks!
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.
Although no it's the correct one. (It works when I view it using mkdocs serve
)
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.
All look ok to me for this PR, except this line, but if you say it's ok, it's a ✔️ from me
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.
LGTM
@awgymer can you also approve this since you requested changes? |
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.
Changes mostly look good to me.
Few small clarifications needed before merge
but I've approved anyway.
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.
The font looks off here (looks like a serif font of some form) but not sure if just a github display issue?
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.
Yeah I'm not sure what's going on there... The png
looks fine though. @mirpedrol any ideas?
Okay I'll merge this now, we can still open another PR if more small issues are found/resolved |
This PR changes all nf-validation mentions to nf-schema (with exception of the changelog)