update scite plugin #415
update scite plugin #415
Conversation
|
AppVeyor build 1.0.226 for commit d4e1895 is now complete. Found 52 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscingcontent/02.delete-me.md:44:aliqua content/02.delete-me.md:44:amet content/02.delete-me.md:44:consectetur content/02.delete-me.md:44:dolore content/02.delete-me.md:44:eiusmod content/02.delete-me.md:44:elit content/02.delete-me.md:44:incididunt content/02.delete-me.md:44:ipsum content/02.delete-me.md:44:labore content/02.delete-me.md:44:Lorem content/02.delete-me.md:44:magna content/02... |
|
AppVeyor build 1.0.227 for commit dd2d243 is now complete. Found 52 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscingcontent/02.delete-me.md:44:aliqua content/02.delete-me.md:44:amet content/02.delete-me.md:44:consectetur content/02.delete-me.md:44:dolore content/02.delete-me.md:44:eiusmod content/02.delete-me.md:44:elit content/02.delete-me.md:44:incididunt content/02.delete-me.md:44:ipsum content/02.delete-me.md:44:labore content/02.delete-me.md:44:Lorem content/02.delete-me.md:44:magna content/02... |
|
What is the expected behavior here? I don't see scite in the PDF but I also don't see it in the HTML |
Because scite is disabled by default |
Can we enable it for review and then disable again before merging? |
|
AppVeyor build 1.0.228 for commit e4fa2a8 is now complete. Found 52 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscingcontent/02.delete-me.md:44:aliqua content/02.delete-me.md:44:amet content/02.delete-me.md:44:consectetur content/02.delete-me.md:44:dolore content/02.delete-me.md:44:eiusmod content/02.delete-me.md:44:elit content/02.delete-me.md:44:incididunt content/02.delete-me.md:44:ipsum content/02.delete-me.md:44:labore content/02.delete-me.md:44:Lorem content/02.delete-me.md:44:magna content/02... |
|
@agitter @cgreene i've temporarily enabled scite to show that it works. Also side note, the badge developer DM'd me the other day and said that the issue where the doi isn't found never returning has been fixed. That is, if they can't find the paper in their system, previously nothing returned and our scite badges would just be stuck on "Loading scite badge", but now they will instead just return all 0's and still show the badge. So we shouldn't have any stuck "loading" messages, and no need for a timeout anymore. |
Note that this doesn't completely disable the scite plugin for printing. See this long PR discussion: greenelab/covid19-review#846 It's an Athena bug. Should we implement the solution there here in rootstock? Idk if it's necessary because 1) scite is disabled by default 2) we might be switching off athena soon? and 3) it's only the appveyor step that has the potential to fail - the badge still gets hidden in print via the CSS but the scite javascript script itself will still run and possibly stall for appveyor (conditions of when this happen are unclear. maybe it's too many references? maybe it's references that aren't in the scite database?). |
|
I think we probably should. Failing via timeout isn't terribly friendly. @agitter what do you think? |
|
Failing on timeouts is pretty bad. Was that only affecting AppVeyor builds and not GitHub Actions? Looking at some of the earlier commits in greenelab/covid19-review#846 and @vincerubinetti's comment above, that may be the case. I'm leaning toward not modifying the rootstock build to use the workaround we added in greenelab/covid19-review#846. The timeout only affects users who are using AppVeyor (i.e. not the default Manubot CI) and who enabled scite, which may be a small pool of affected users. If we always rebuild the HTML a second time for Athena, it will slow default builds for all users. It also creates mostly redundant HTML pandoc setting files the way we currently set it up (which could be improved). My preference is to add a comment next to the scite plugin in |
|
The failures were appveyor specific |
I agree with this. I say keep it simple until it affects way more users.
Regarding this, I want to emphasize that we're not entirely clear what is causing the problem. Casey, in the slew of different commits/workflow runs/PRs, I forget, did we actually try a version with a timeout that removes the Scite badge after ~30 sec? Did that stop AppVeyor from crashing? It may be too many references. It may be having a particular reference that isn't found in their database. Maybe the problem won't happen anymore after their recent update to the Scite badge. Maybe my test idea to have a timeout to remove the badge doesn't even work (it just removed the HTML node for the badge, it didn't do anything to stop the Scite third-party script). |
|
AppVeyor build 1.0.229 for commit 0d29628 is now complete. Found 52 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscingcontent/02.delete-me.md:44:aliqua content/02.delete-me.md:44:amet content/02.delete-me.md:44:consectetur content/02.delete-me.md:44:dolore content/02.delete-me.md:44:eiusmod content/02.delete-me.md:44:elit content/02.delete-me.md:44:incididunt content/02.delete-me.md:44:ipsum content/02.delete-me.md:44:labore content/02.delete-me.md:44:Lorem content/02.delete-me.md:44:magna content/02... |
There was a problem hiding this comment.
Looks good to me. Based on the questions @vincerubinetti raises about not fully understanding the source of the AppVeyor timeouts, and the rarity of the AppVeyor + scite use case, I'm happy to defer any timeout workarounds until we know more.
|
AppVeyor build 1.0.230 for commit 9067f45 is now complete. Found 52 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscingcontent/02.delete-me.md:44:aliqua content/02.delete-me.md:44:amet content/02.delete-me.md:44:consectetur content/02.delete-me.md:44:dolore content/02.delete-me.md:44:eiusmod content/02.delete-me.md:44:elit content/02.delete-me.md:44:incididunt content/02.delete-me.md:44:ipsum content/02.delete-me.md:44:labore content/02.delete-me.md:44:Lorem content/02.delete-me.md:44:magna content/02... |
|
Feel free to merge |
* Webpage: refactor plugins & add scite plugin merges manubot/rootstock#409 * moves repeated and shared/generic functions to separate "core" plugin * reorganizes html.yaml config into first and third party plugins * removes functionality to set plugin options from url * reformats plugins with Prettier (eg 4 space tabs to 2 space) * removes anonymizer wrapper. Just make <script> tag into module to keep scope local and avoid function name conflicts. this reduces the indent of the whole script by one level. * moves plugin specific CSS to plugins themselves * adds scite plugin (uncomment to enable) * Update scite plugin merges manubot/rootstock#415 * setup.bash: interactive script to guide setup merges manubot/rootstock#417 closes manubot/rootstock#401 * Add "gh repo create" to SETUP.md merges manubot/rootstock#419 closes manubot/rootstock#418 Co-authored-by: Daniel Himmelstein <[email protected]> Co-authored-by: Anthony Gitter <[email protected]> * fix link Co-authored-by: Vincent Rubinetti <[email protected]> Co-authored-by: nfry321 <[email protected]> Co-authored-by: Tiago Lubiana <[email protected]> Co-authored-by: Daniel Himmelstein <[email protected]> Co-authored-by: Anthony Gitter <[email protected]>
onscrollcruft