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

Show sample citations & bib entries for PRs that validate #13

Open
adam3smith opened this issue Apr 26, 2019 · 77 comments

Comments

Projects
None yet
4 participants
@adam3smith
Copy link
Member

commented Apr 26, 2019

Just going to do a simple mock-up:
Currently Sheldon posts this when a PR gets opened:

Awesome! You just created a pull request to the Citation Styles Language styles repository. One of our human volunteers will try to get in touch soon (usually within a week). In the meantime, I will run some automated checks. You should be notified of the results in a few minutes.

If you haven't done so yet, please make sure your style validates and follows all our other Style Requirements.

To update this pull request, visit the "Files changed" tab above, and click on the pencil icon (see below) in the top-right corner of your style to start editing.

If you have any questions, please leave a comment and we'll get back to you. While we usually respond in English, feel free to write in whatever language you're most comfortable.

Followed by this when the PR passes our tests:

😃 Your submission passed all our automated tests.

I'd suggest leaving the first message as is, but would like the second one to be something like:

😃 Your submission passed all our automated tests.

Your style will produce the following output:
Citations: (Campbell & Pedersen, 2007; Mares, 2001)

Bibliography:
Campbell, J. L., & Pedersen, O. K. (2007). The varieties of capitalism and hybrid success. Comparative Political Studies, 40(3), 307–332. https://doi.org/10.1177/0010414006286542
Mares, I. (2001). Firms and the welfare state: When, why, and how does social policy matter to employers? In P. A. Hall & D. Soskice (Eds.), Varieties of capitalism. The institutional foundations of comparative advantage (pp. 184–213). New York: Oxford University Press.

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.

@rmzelle

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

(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

image
)

@retorquere

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

What would be the benefit to pulling the data from a public group vs having it in the text fixtures?

@adam3smith

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

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 GET call.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Alright, no major difference between the two anyhow. So, the main ask here is to

  1. Render a citation/bibliography with the PR
  2. Render a citation/bibliography from master
  3. Diff them

(with the rendering done using citeproc-js I assume)

?

@adam3smith

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Yes, I think that's basically it. I think we'll only want to do this when the other tests pass, though.

@retorquere

This comment has been minimized.

Copy link
Contributor

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.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

What is the relation between the spec tests in the styles repo and the test-suite repo?

@retorquere

This comment has been minimized.

Copy link
Contributor

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.

@adam3smith

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

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.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Any thoughts on what to use as the items to render?

@adam3smith

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Here's a set of four that I like (recycled from the CSL editor) https://gist.github.com/adam3smith/7f0c65f116d5e17df0c23901198f42c7

@retorquere

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

It's been a while since I used Ruby -- how are describe et al brought into the current namespace in styles_spec.rb? I intended to use an after(:all) hook, but rspec complains it can't find after.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Ah never mind, I can just use travis hooks.

@retorquere

This comment has been minimized.

Copy link
Contributor

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)

@adam3smith

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

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

@retorquere

This comment has been minimized.

Copy link
Contributor

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.

@retorquere

This comment has been minimized.

Copy link
Contributor

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.

@adam3smith

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Here are two that would show up in diffs, though I think only for the journal article in both cases:
citation-style-language/styles#4037
citation-style-language/styles#4064

@adam3smith

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

I like the quoted look -- once we add some surrounding text, I think that'll be great.

@retorquere

This comment has been minimized.

Copy link
Contributor

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.

@retorquere

This comment has been minimized.

Copy link
Contributor

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.

@retorquere

This comment has been minimized.

Copy link
Contributor

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.

@rmzelle

This comment has been minimized.

Copy link
Member

commented May 18, 2019

there are two sets of styles being tested, and I don't understand the difference between them.
...
I mean I see tests on "dependents" and "independents" but I don't know what those terms mean in this context.

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.

differ is the only one that I could find that did char-by-char coloration of diffs.

jsdiff does too (although it currently can't diff markup differences). See http://incaseofstairs.com/jsdiff/.

@rmzelle

This comment has been minimized.

Copy link
Member

commented May 18, 2019

It still looks a little crowded to me

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)
(Fenner et al., 2019; Mares, 2001)

CSL search by example. (2012). Retrieved December 15, 2012, from Citation Style Editor website: http://editor.citationstyles.org/searchByExample/
Fenner, M., Crosas, M., Grethe, J. S., Kennedy, D., Hermjakob, H., Rocca-Serra, P., … Clark, T. (2019). A data citation roadmap for scholarly data repositories. Scientific Data, 6(1), 28. https://doi.org/10.1038/s41597-019-0031-8
Hancké, B., Rhodes, M., & Thatcher, M. (Eds.). (2007). Beyond varieties of capitalism : Conflict, contradiction, and complementarities in the European economy. Oxford and New York: Oxford University Press.
Mares, I. (2001). Firms and the welfare state: When, why, and how does social policy matter to employers? In P. A. Hall & D. Soskice (Eds.), Varieties of capitalism. The institutional foundations of comparative advantage (pp. 184–213). New York: Oxford University Press.

(using <details/> per https://gist.github.com/joyrexus/16041f2426450e73f5df9391f7f7ae5f and dear-github/dear-github#166)

@retorquere

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

Are you familiar with dependent CSL styles at all?

Nope.

jsdiff does too (although it currently can't diff markup differences). See http://incaseofstairs.com/jsdiff/.

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.

@retorquere

This comment has been minimized.

Copy link
Contributor

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.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

Still working on getting the rspec failure output

@rmzelle

This comment has been minimized.

Copy link
Member

commented May 19, 2019

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.:

apa.csl (modified style; unchanged output for sample items)
international-journal-of-climatology.csl (modified style; changed output)
junk.csl (new style)

(@adam3smith, let me know if you think that's an improvement)

@rmzelle

This comment has been minimized.

Copy link
Member

commented May 19, 2019

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:

  1. if you reach the limit of styles to render (per above), give preference to independent CSL styles over dependent styles for the styles you do render.
  2. We also sometimes replace an independent style by a dependent one, and vice versa. So it would be good if e.g. a replacement of https://github.com/citation-style-language/styles/blob/master/dependent/nature-biotechnology.csl by https://github.com/citation-style-language/styles/blob/master/nature-biotechnology.csl would be recognized as a modified style (the style ID and file name would stay the same in these cases).
@retorquere

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

I've put up a new sample.

It turns out it is possible to do inline diffs using <del> and <ins> (which GH does render) but they cannot be colorized and I found them really hard to spot in the output.

There's currently no limit; all changed styles are rendered. WRT dependents/independents:

  • How do I get Independents and Dependents into my script? What should I require?
  • Will they pick up new styles on disk, or do they only list items from whatever gem that hosts them?
  • If an independent style changes, should I be checking all its dependents?

I will look at your point 2.

@rmzelle

This comment has been minimized.

Copy link
Member

commented May 25, 2019

There's the other issue though that the sheldon gem now relies on files being in specific places in another repo. I don't mind, personally.

I don't think this really matters for us as long as there is some documentation somewhere on what file-dependencies there are.

What did/should sheldon report/do for the locales repo?

See the three comments by @csl-bot in e.g. citation-style-language/locales#188. We'd have a welcome comment, and for each commit a success or failure comment. We'd want to insert the test failures directly again here as well, instead of just linking to the Travis CI report.

It would be good if your implementation would still provide links to the Travis CI reports, by the way, for both "styles" and "locales" repositories (e.g. underneath the test with a "(see for the full Travis CI test report)".

@retorquere

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Gemification done:

@retorquere

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

I've added a README to the Sheldon branch that explains the expectations it has on the styles and locales repos; all files live in spec/sheldon in those respective repos currently, and the messages Sheldon will post can be edited there.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

I'm terribly sorry about this, but I had forgotten that in PRs, secret variables like github tokens are not set (for good reasons). This means PR-builds cannot post directly to GH. I've been talking to Travis support about this (who are super responsive!), and it looks like the best option is to mostly keep Sheldon but also keep the stuff I've done and have them cooperate.

The reason now-Sheldon can't (reasonably) do everything on its own is that you really need a checked out repo at the exact point where the PR runs as well as access to the master branch and have the travis vars available -- we'd be trying to replicate the Travis build environment on Heroku (which is where shel-bot runs right?)

The coop solution would be to keep shel-gem mostly as-is, but have it not post to github but either upload its output to transfer.sh or just output it to the console. shel-bot would then parse out either the transfer.sh response url or the shel-gem output from the travis log, and post that.

There's upsides and downsides to each of these coop modes:

  • transfer.sh would look cleanest (it's just a simple URL), but it puts a service between the two sides of sheldon, which may be fragile.
  • Outputting the comments would be most robust, but they would be user-visible, and not in a format that would necessarily be nice to look at (a mixture of raw HTML and markdown).

I don't currently see a way around this. I'm trying to think of a format which would look good enough to post in the log that would also be re-parsable for shel-bot.

@adam3smith

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

I think it makes sense to go with the transfer.sh option and see if we're having issues with stability. Given that one of the reasons for doing this is to improve the experience for style contributors, clean output would be important.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

I'm trying a few services - transfer.sh is really slow (10-20 seconds) and in some of my tests it simply times out when I try to post the assets. Edit: this may be a rate limit because I'm now structurally hitting this but I've also occasionally seen transfer.sh just keel over.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

0x0.st explicitly forbids "spamming the service with CI build assets" and file.io has both a monthly limit of 100 uploads (and it's probably enforced by IP address so from a CI service we'd be pooled in with anyone else using it) and there's some kind of rate limit going on (I sometimes get "too many requests" even from my home system).

If anyone knows other services of the kind, I'd love to hear about it, but I'm currently looking for different solutions altogether.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

One other option would be to set up an S3 or backblaze bucket which allows anon uploads but only authenticated downloads, with an auto-expiry policy to keep the bucket size down. I use this setup for BBT logs and I've only once broken the 8ct/month limit which was when I was furiously putting out new builds in the Z5 port. Benefit of this would be that there'd need to be nothing in the logs -- shel-bot could just pick it up at an agreed name.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

There's one other way that just struck me -- technically possible but ho-hum from a beauty point of view. The travis log renderer honors ansi escape sequences, so I could output the shel-bot message, erase it using backspaces, which would remove it from view but it would remain in the actual log where I could parse it out.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

There's an ungodly amount of magic going on in rspec -- I'm getting

undefined method `assert_equal' for nil:NilClass

when I run rspec on shel-bot. I've so far traced it back to the minitest context (ctx) being nil in minitest-5.11.3/lib/minitest/spec.rb. Anyone here experience with rspec?

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

In a clean rvm setup, I'm hitting

$ rspec
/Users/emile/.rvm/rubies/ruby-2.4.1/lib/ruby/site_ruby/2.4.0/rubygems.rb:283:in `find_spec_for_exe': can't find gem rspec-core (>= 0.a) with executable rspec (Gem::GemNotFoundException)
	from /Users/emile/.rvm/rubies/ruby-2.4.1/lib/ruby/site_ruby/2.4.0/rubygems.rb:302:in `activate_bin_path'
	from /usr/local/lib/ruby/gems/2.6.0/bin/rspec:23:in `<main>'
@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

Wait, I don't need rspec -- I just need to run rake. But when I run rake, I get

Fabulous run in 0.000468s, 0.0000 runs/s, 0.0000 assertions/s.

0 runs, 0 assertions, 0 failures, 0 errors, 0 skips
@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

This is driving me nuts. What I see in the Rakefile seems to use minitest, but then the actual spec files use rspec's describe/it syntax. @inukshuk, can you give me a push in the right direction?

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

Alright, more clarity; the csl gems pull in newer versions of rspec it seems, and the existing test setup does not like this. No errors, but no tests ran.

The quick way to solve this is to keep shel-bot and shel-gem on different branches. I do worry a little about the technological debt shel-bot is going to accumulate over time. I had tried to merge shel-bot and shel-gem (which should be possible), but realistically, that would require some gems in shel-bot to be updated, and due to the amount of magic going on in rspec, it's not easy to find out which gem is injecting what into the various namespaces.

@inukshuk

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

@retorquere I've not been following along, but on a quick glance Sheldon does not use rspec but rather minitest. The csl/citeproc gems use rspec, but only as dev dependencies -- those should not be pulled in when you install them (unless you specifically use dev dependencies).

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

My bad.

I've been able to get it to work by pinning gems to the exact versions I get when the tests run on Travis. I've not been able to get the tests to run bu just having the gems all at their latest versions -- rake runs without errors but no tests are actually ran.

If this pinning is OK, I can proceed to merge the shel-gem part; it doesn't actually touch shel-bot (yet), just adds the gem-installable executable to the existing Sheldon so that the travis counterpart can be easily installed on styles and locales.

@inukshuk

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

I'm a bit confused by that gemspec file. Most gems don't package test/spec files at all, in my experience; the test files also don't seem to be added to spec.files here, so maybe that's why no tests ran? The file also pulls in rspec even though it's not being used, that doesn't seem right. In any case, pinning versions of dev and test dependencies is much easier to do using bundler / Gemfile.lock -- I'd really try to use that instead of specifying everything as gem dependencies. Specifying something like puma is also potentially restricting since you could use other rack-compatible servers to run this. Furthermore, the bin folder` only contained bundler binstubs, which are just there for convenience really, it's not something that should be included in a gem.

That said, I'm really out of the loop here, so I may be missing the point.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

Ah no, the gemspec file isn't complete yet, but for the shel-bot part nothing changes; the gemspec is really only being used by gem consumers. I can run rake in the current state and the tests run, it's just when I upgraded the gems that the tests don't run. I don't intend to add the spec files to spec.files.

Using the gemspec is really the same as using the Gemfile from the point of view of shel-bot -- a lockfile is also generated as normal. That said, I'm no expert on gem packaging, I'm just cribbing things together best I can.

Turns out puma isn't required, I just at some point added everything that I saw was being pulled in previously to just get the tests going. The tests run without puma being brought in just fine, so I've removed it. I'm going through the pinned gems one by one to see which I can remove from pinning.

The bin directory also include the executable I intend to run on Travis, so it's no longer just the stubs.

In the end, the idea is that the gem-part offers a ruby script that generates a GH-friendly summary of the test results, which the bot-part can pick up. I could just have either the gem-part on a separate branch, or even a separate repo, but given that these two would work so closely together, at least the same repo would make most sense to me.

@inukshuk

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Is the part that's going to be run on Travis also run by the stateless web service on Heroku?

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

No, but the part that's going to be ran on heroku will expect there to be something in the travis log that's put there by the script in bin.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

I'm going to start moving stuff back into the Gemfile -- I had misinterpreted the gemspec stuff I found to say that everything needs to be there, which is not the case.

@inukshuk

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Cool, I think that's better (I'd just leave runtime deps in the gemspec). If the Travis script does not run on Heroku, I'd also put its dependencies into a separate Group in the Gemfile which is not installed on Heroku. But in general, maybe it would be easier to use a separate repo for this (because I'm not sure that you could actually keep dependencies defined in the gemspec file out of the bundle created on Heroku).

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

It may be possible to have the gem deps not installed on heroku; the current gemfile adds a section called travis (https://github.com/retorquere/Sheldon/blob/gem/Gemfile#L29), but I don't know how heroku chooses what sections to install.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

(other than that it turns out I only had to pin hashdiff because newer versions of hashdiff have a namespace conflict with another gem).

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

OK, I think I have things prepped now. Shel-bot should be unaffected other than picking up the build report from the log, and the sheldon gem now lives in a single script. I'm going to deploy it to my own heroku instance to see how it works out.

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

Two questions:

  1. How can I verify that my copy of shel-bot is receiving events and acting on them? I have my copy set up on https://retorquere-sheldon.herokuapp.com/ but I'm not seeing any responses on my own PR.
  2. Perhaps related: it looks like PRs submitted by the repo owner do not get treated as PRs; @adam3smith, could you merge the reporting branch again and push to see if that's the difference?
@rmzelle

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

  1. How can I verify that my copy of shel-bot is receiving events and acting on them? I have my copy set up on https://retorquere-sheldon.herokuapp.com/ but I'm not seeing any responses on my own PR.

Maybe you're missing a webhook (https://github.com/citation-style-language/styles/blob/0e4b1f80ed20ccf3a88febebf2c526aa734d80e1/.travis.yml#L14)?

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

I have it figured out (and tested), the PRs are ready.

@adam3smith

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

I have it figured out (and tested), the PRs are ready.

... and waiting for your input: #15 (comment)

@retorquere

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

OK, I think everything is in place now. Sheldon is running on heroku-18, and styles/locales now automatically fetch the latest sheldon reporter during the tests, so maintenance can be done on this repo for the entire setup. I think that is that then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.