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

Adding PLOS info, changing schema #11

Closed
wants to merge 5 commits into from

Conversation

jpolka
Copy link
Member

@jpolka jpolka commented May 10, 2018

No description provided.

@dhimmel
Copy link
Member

dhimmel commented May 10, 2018

Looks like the build failure is from:

ruamel.yaml.scanner.ScannerError: mapping values are not allowed here

Will look into what this is tomorrow.


# Peer review policy url (valid url)
peer-review-url:
peer-review-url: http://journals.plos.org/plosone/s/editorial-and-peer-review-process
Copy link
Member

Choose a reason for hiding this comment

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

I think the build is failing because the YAML is incorrect. Strings containing : need to be quoted I think. Try quotes around URLs like:

peer-review-url: "http://journals.plos.org/plosone/s/editorial-and-peer-review-process"

Perhaps a YAML linter could help us detect these formatting problems.

Copy link
Member

Choose a reason for hiding this comment

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

According to adrienverge/yamllint#30 (comment) the colon in the URL may actually be okay, but let's see if the issues are resolved when we quote URLs.


# What are the titles of the sections of the review form? (free text)
review-structure:
review-structure: Is the manuscript technically sound? (drop down) Has the statistical analysis been performed appropriately and rigorously? (drop down) Does the manuscript adhere to the PLOS Data Policy? (drop down) Is the manuscript presented in an intelligible fashion and written in standard English? Review comments to the author. If you would like your identity to be revealed to the author, please include your name here (optional). COI disclosure to the editor. Did you recieve any assistance in preparing the review (eg from a post-doc or graduate student? If yes, please provide his/her name and title here. Reference: http://journals.plos.org/plosone/s/file?id=t6Vo/plosone-reviewer-form.pdf
Copy link
Member

Choose a reason for hiding this comment

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

Ah this is probably the issue: Reference:

jpolka added 2 commits May 10, 2018 05:35
Added two missing url fields and rearranged (!?!!) some into a new heading, "peer review form"
@jpolka
Copy link
Member Author

jpolka commented May 10, 2018

I've revised the schema, including moving "co-review-field" under a new heading called "peer review form" and see that this causes a conflict - presumably it would be easier to just keep it where it is?

@jpolka jpolka changed the title Adding PLOS info Adding PLOS info, changing schema May 10, 2018
@dhimmel
Copy link
Member

dhimmel commented May 10, 2018

I've revised the schema, including moving "co-review-field" under a new heading called "peer review form" and see that this causes a conflict - presumably it would be easier to just keep it where it is?

If we are going to update the schema, not is the best time. It could get harder once we have files that are curator populated.

I will see if I can take this change and apply it.

Then I will help you fix this PR and go over what went wrong.

@dhimmel
Copy link
Member

dhimmel commented May 10, 2018

At least d1bb385 produced a different build failure, so the original issue has been fixed. Here's the new error:

core.py                    164 ERROR    ["Value 'http://journals.plos.org/plosone/s/editorial-and-peer-review-process' does not match pattern '/^(TODO|(ht|f)tp(s?)\\:\\/\\/\\w[\\/\\.\\-\\:\\=\\?\\&\\_\\+\\u0023\\w]+)$/'. Path: '/peer-review-url'", "Value 'http://journals.plos.org/plosone/s/reviewer-guidelines#loc-confidentiality' does not match pattern '/^(TODO|(ht|f)tp(s?)\\:\\/\\/\\w[\\/\\.\\-\\:\\=\\?\\&\\_\\+\\u0023\\w]+)$/'. Path: '/co-review-url'", "Value 'yes' is not of type 'bool'. Path: '/co-review-field'", "Value 'yes' is not of type 'bool'. Path: '/separate-structure'", "Value 'http://journals.plos.org/plosone/s/criteria-for-publication' does not match pattern '/^(TODO|(ht|f)tp(s?)\\:\\/\\/\\w[\\/\\.\\-\\:\\=\\?\\&\\_\\+\\u0023\\w]+)$/'. Path: '/preprint-url'"]

Looks like the URL pattern could have a bug. I will investigate.

@dhimmel
Copy link
Member

dhimmel commented May 10, 2018

Okay I think took the schema changes from here and applied some additional ones to the URL patterns in #13. Once we merge that, we can create a PR with only the PLOS policy updates.

@jpolka
Copy link
Member Author

jpolka commented May 10, 2018

Looks like the URL pattern could have a bug. I will investigate.

Great, thank you - in addition, are the bool values not evaluating properly? ie

"Value 'yes' is not of type 'bool'.

Are these the valid ones? http://yaml.org/refcard.html Also I notice that we use "no" without quotes as a possible str value in some of the early fields - should we change that? If any of the above are true, we should probably change the descriptions to prompt users with the proper capitalization etc (and harmonize capitalization of expected responses throughout)

@dhimmel
Copy link
Member

dhimmel commented May 10, 2018

It looks like in YAML v1.2 the only support boolean values are true and false (spec). This is a good change IMO since it can be confusing when things meant to be strings get converted to bools. I believe we are using a 1.2 parser, but I can double check. So our options are:

  1. Keep bool fields and rewrite descriptions to use true/false
  2. Switch bools back to an enum string with yes or no.

@jpolka do you have a preference?

@jpolka
Copy link
Member Author

jpolka commented May 10, 2018

Hmm - I feel like yes/no is a little more natural? Unless there are advantages to booleans maybe we could stick with that?

@dhimmel
Copy link
Member

dhimmel commented May 10, 2018

Sounds good. I've taken the PLOS related commits from this PR and moved them to a new PR in #14

@jpolka jpolka closed this in #14 May 10, 2018
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