Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upShow sample citations & bib entries for PRs that validate #13
Comments
This comment has been minimized.
This comment has been minimized.
(and, as a second optional layer on top, a diff versus an appropriate comparison style could also be very useful, e.g. using something like https://github.com/kpdecker/jsdiff, which can generate output such as |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 6, 2019
What would be the benefit to pulling the data from a public group vs having it in the text fixtures? |
This comment has been minimized.
This comment has been minimized.
This was purely pragmatic -- having them in the fixture is fine too. I was thinking of going through the Zotero API as one option, and for that purpose having them items available in a group makes getting the bib possible with a single |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 6, 2019
Alright, no major difference between the two anyhow. So, the main ask here is to
(with the rendering done using citeproc-js I assume) ? |
This comment has been minimized.
This comment has been minimized.
Yes, I think that's basically it. I think we'll only want to do this when the other tests pass, though. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 9, 2019
Right. I'm making some headway, but citeproc is a little sparsely documented, so I'm going through their test cases to piece things together. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 9, 2019
What is the relation between the spec tests in the |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 11, 2019
Alright, I have diffs rendering, but citeproc in its various incarnations could really do with better documentation. Oy vey. I've stuck to citeproc-ruby because the output really shouldn't differ from citeproc-js and the existing tests used Ruby. |
This comment has been minimized.
This comment has been minimized.
Sorry, hadn't seen the question above: the test-suite tests correct citeproc behavior and has no relationship to the styles repo. Every test includes its own style. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 13, 2019
Any thoughts on what to use as the items to render? |
This comment has been minimized.
This comment has been minimized.
Here's a set of four that I like (recycled from the CSL editor) https://gist.github.com/adam3smith/7f0c65f116d5e17df0c23901198f42c7 |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 16, 2019
It's been a while since I used Ruby -- how are |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 16, 2019
Ah never mind, I can just use travis hooks. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 16, 2019
I'm still not happy with the way things look when added as a comment. I've put up a sample here (apologies for the junk PR I accidentally opened on the actual styles repo) |
This comment has been minimized.
This comment has been minimized.
What are you not happy with? You could maybe quote it as in the examples above, but I think functionally this is very nice already |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 16, 2019
I've added a 2nd sample that quotes it. It still looks a little crowded to me, but if it's good enough for you guys, that's what counts. I'm still playing with diffy to output decent looking diffs, but GH comments are pretty narrow and it gets unreadable fast. Differ does inline colored diffs which are easier to read, but the library hasn't seen updates in 8 years, and issues on the repo don't get picked up. I'm hesitant to bake in a tech debt, OTOH, it does work. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 16, 2019
Can anyone point me to a style that changed in a way that the sample references might show a difference in rendering? The random samples I looked at did stuff like et-al fixes that wouldn't trigger on the number of authors in the samples, or for other reasons showed no differences. |
This comment has been minimized.
This comment has been minimized.
Here are two that would show up in diffs, though I think only for the journal article in both cases: |
This comment has been minimized.
This comment has been minimized.
I like the quoted look -- once we add some surrounding text, I think that'll be great. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 16, 2019
Alright - you could start thinking about what you want to show when the render differs and when not. I could perhaps have the rspec tests only test the changed files in a PR, but there are two sets of styles being tested, and I don't understand the difference between them. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 16, 2019
Alright, I've added a new sample with 3 changes; one where a style changed but the output did not, one where the style changed and the output also changed, and one where a new style was added. Old or not, differ is the only one that I could find that did char-by-char coloration of diffs. Thoughts? Also thoughts on what the diff should diff? Text diffs are going to be more readable than markup diffs, but then markup changes would be lost in the report. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 16, 2019
I mean I see tests on "dependents" and "independents" but I don't know what those terms mean in this context. |
This comment has been minimized.
This comment has been minimized.
Are you familiar with dependent CSL styles at all (like https://github.com/citation-style-language/styles/blob/master/dependent/nature-biotechnology.csl)? Dependent CSL styles inherit their style format from the referenced "independent-parent", and are heavily used for large publishers like Elsevier and Springer Nature that have thousands of journals that only use a handful of distinct citation formats. They're stored in the "dependent" directory of the "styles" repo. See also https://docs.citationstyles.org/en/stable/specification.html#file-types.
jsdiff does too (although it currently can't diff markup differences). See http://incaseofstairs.com/jsdiff/. |
This comment has been minimized.
This comment has been minimized.
Especially if we envision rendering citations for every commit in a pull request, we might also want to collapse things a bit, e.g.: Changed: apa.csl
(“CSL search by example,” 2012; Hancké, Rhodes, & Thatcher, 2007) CSL search by example. (2012). Retrieved December 15, 2012, from Citation Style Editor website: http://editor.citationstyles.org/searchByExample/
(using |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 18, 2019
•
Nope.
Turns out GH comments don't allow coloration. It's possible to do line-by-line coloration by using the diff format, but from my experience that's only of limited use for CSL styles, because I figure you'd generally want to know what changed in the line. But it's all we're going to get. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 18, 2019
I've put up two new samples, one which does line-by-line diffs of html, the other of markdownified html. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 18, 2019
Still working on getting the rspec failure output |
This comment has been minimized.
This comment has been minimized.
I like the line-by-line diffs of HTML best. HTML tags are easier to spot than markdown markup. Maybe you could tweak the summary labels a little as well? E.g.:
(@adam3smith, let me know if you think that's an improvement) |
This comment has been minimized.
This comment has been minimized.
And did you already put a limit on the number of styles you're rendering? It's not uncommon to have the occasional pull request that touches several hundred or even thousands of styles, so limiting the render to e.g. no more than 10 styles would be a good idea. And assuming you'll be handling dependent CSL styles as well, it might be good to:
|
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 19, 2019
•
I've put up a new sample. It turns out it is possible to do inline diffs using There's currently no limit; all changed styles are rendered. WRT dependents/independents:
I will look at your point 2. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 19, 2019
Does anyone here know what |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 19, 2019
I've just taken (in)dependence from the path names. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 19, 2019
Currently set up so that if the tests fail, it will post the failure, if they pass, at most 10 styles are rendered, with independents taking precedence, and it will tell you how many there actually were. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 19, 2019
The current script lives on https://github.com/retorquere/styles/tree/reporting; the files of interest are:
|
This comment has been minimized.
This comment has been minimized.
from https://stackoverflow.com/a/35142442/1483360
So makes perfect sense that a pure rename is 100% similar and a rename&mod is some number <100% similar. |
This comment has been minimized.
This comment has been minimized.
I'm really happy with this as is -- we'd obviously want to add brief messages for the contributors to make sense of this, but in terms of the output I would just take this as in the last example. @rmzelle -- are you good to go with the current version? I think just adding explanatory text during the merge process might be easiest so Rintze and I can debate the details there.. |
This comment has been minimized.
This comment has been minimized.
@adam3smith, I agree this looks great. With a few more changes it looks like we can retire Sheldon entirely, right? I think the only thing we're missing right now is the welcome comment after a PR is opened (and support for the "locales" repo). @retorquere, with regard to the failed tests, it would be very nice if we could clean up the rspec output a little. I don't know if this can be configured directly within rspec, but the following lines should ideally be stripped from the output as they're not informative to the typical contributors of CSL styles:
The final block can also be deleted:
This would reduce retorquere/styles#1 (comment) to: Tests failed
Finally, a link to the build report would also be handy, and I see that some HTML/XML tags are currently not being escaped properly: |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 20, 2019
The script can also post the welcome message, and I currently see a few ways to go about this; the script would scan the PR comments upon start
As to trimming the output, I am looking at the rspec json logger to see if I can make sense of its output. It's possible to write a custom formatter to do whatever we want directly (such as generating markdown), but I could not find any documentation on this saying anything else than "it's possible". |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 20, 2019
New sample of test failure log is up. |
This comment has been minimized.
This comment has been minimized.
@retorquere, I think (3) makes the most sense. We currently use https://github.com/csl-bot to post the Sheldon comments (which is actually under control of Zotero), and we might be able to co-opt that account. And where did you make the changes that resulted in retorquere/styles#1 (comment)? https://github.com/retorquere/styles/tree/reporting hasn't seen an update since 5/19. Would you also be open to supporting the "locales" repo? We use Sheldon in very similar capacity there (just with separate templates: https://github.com/citation-style-language/Sheldon/tree/master/templates). We probably wouldn't need any citation rendering there, but it would e.g. be nice to have the GitHub comments with test failure, and it would allow us to retire Sheldon altogether. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 23, 2019
I forgot to push the latest changes, they're now up. Yeah sure, I haven't looked at the locales repo, but if it substantially does the same thing, that seems sensible. I'll have a look. I'll add the scanning code for point 3. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 24, 2019
Turns out it's not too hard to make a github-installable gem to share the code. Would you guys prefer the test-assets (like |
This comment has been minimized.
This comment has been minimized.
I don't have a strong opinion but my first reaction would be to leave them in the respective repos because Rintze and I have those cloned locally already, so easier to update. But if there's any reason to keep them in the gem (speed, some other best practice reason), this is a minor consideration and I'd be happy with that, too. |
This comment has been minimized.
This comment has been minimized.
retorquere
commented
May 24, 2019
There's no real benefit, but I saw that Sheldon also had local assets for styles and locales, and perhaps the |
adam3smith commentedApr 26, 2019
•
edited by rmzelle
Just going to do a simple mock-up:
Currently Sheldon posts this when a PR gets opened:
Followed by this when the PR passes our tests:
I'd suggest leaving the first message as is, but would like the second one to be something like:
Where citations and bibliography are generated from the new style (I was thinking we could have the data in a public Zotero group and simply use API calls to Zotero, but agnostic about the method to be used.