-
Notifications
You must be signed in to change notification settings - Fork 284
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 NCBI-PD #2433
Add NCBI-PD #2433
Conversation
First please wait till the issue is approved. |
Moving PR to draft. |
f0e2dd4
to
1409961
Compare
Requesting @swinslow for review, re-labeling, and adding the milestone |
40b76a2
to
42f530a
Compare
I found a few more variations of the text, ergo the commits. I'd test it locally but it doesn't work for some reason. |
@swinslow - can you review? I think some of the markup we usually do a bit differently |
af50be1
to
368a3a8
Compare
I can't figure out why it's failing. Any tips? |
d39aace
to
d0f2a5a
Compare
Added the title; it's still failing with what appears to be the same error. If you'd like to take over this PR, I'd totally understand. Otherwise, got any ideas as to why it's failing? Everything seems to be correct, but for some reason the regexes aren't matching correctly. |
I'm having a go at updating this in small increments. Also adding some line breaks to make it easier to "see" what is going on. |
ok, so there were a bunch of optional tags that shouldn't be there. Also, I don't see any variation in the Github search that @swinslow did that has "created" instead of "written" - we only add markup for variations actually found (and sparingly). But if I missed something, let me know! Also, given that all of these have the same name in them (National Center for Biotechnology Information / NLM) - I'd be inclined to not mark that up at this point. We can always add markup if we find variations later |
there was the following markup: However, I don't see any variation of this sort, nor did @swinslow mention this. Did I miss something? |
ok, last item to consider: the FAQ added at the end as optional - I only see this in one instance. I'd be inclined to just leave it out. It's not part of the license in any case and I think scanning tools can sort this one out |
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.
Technically speaking the alt matches can be simplified, but this way it is more readable. So +1 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; if needed, we can tweak the markup again to add additional variations for future releases. Thank you all!
Closes #2419