APA overhaul #3602
APA overhaul #3602
Conversation
Fixes regression in citation-style-language#3590
- Also some other minor changes in preparation of a unified format macro
- And show translators, etc. for webpages and blogposts
To meet all of the various requirements Needs testing Need to check the rules for: - patent - treaty - song - interview
- Update legal_cites (bill, legal_case, legislation, treaty) - Update patents - Add additional creators alongside "author"
|
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. |
|
😃 Your submission passed all our automated tests. |
|
Happy to discuss some of the decisions that had to be made. Specific areas that were challenging were accommodating:
I added a few additional hard-coded strings (marked as in need of localization). Most of them should come up fairly rarely I suspect, but we can consider some alternative options if we want to break localization less until more terms can be added to CSL. |
|
Question: Is there any way to get a Produces: Rather than: I had thought this code would produce the latter output (essentially all of the types inside a single At minimum, it seems like there should be a way to specify that |
|
No -- every |
|
Here’s a more common example: If I have a chapter in a book with an editor and a translator (different people), then the existing APA style renders as: Rather than with the ampersand: This CSL behavior seems very counter intuitive to me. Particularly frustrating is that |
Including multiple creator types in one names string with consistent delimiter/and/et-al results isn't currently possible in CSL.
|
(But thanks for the clarification!) |
|
😃 Your submission passed all our automated tests. |
|
😃 Your submission passed all our automated tests. |
|
😃 Your submission passed all our automated tests. |
`illustrator` currently isn't be rendered at all, probably due to a citeproc-js bug
|
😃 Your submission passed all our automated tests. |
|
😃 Your submission passed all our automated tests. |
|
@adam3smith Okay, I've run through a bunch of tests, and everything is looking good on my end. Once you've had a chance to review, I will port the changes over to the other APA versions. |
|
😃 Your submission passed all our automated tests. |
There was a problem hiding this comment.
Some comments/questions, some things that definitely need fixing. More generally, do you have a test set of references you could share. This change is pretty massive & is going to influence lots of users, it'd be good to have a second pair of eyes on the actual testing rather than just the code.
|
Oh, and of course thanks for doing this in the first place. (And so far I've only reviewed the APA changes. |
|
😃 Your submission passed all our automated tests. |
|
Okay, all 251 items in the test library render correctly, or as correctly as they reasonably can. https://www.zotero.org/groups/2205533/test_items_library In the library, items that produce output that deviates from the APA manual are tagged with It should be ready for your review now @adam3smith |
|
😃 Your submission passed all our automated tests. |
|
@adam3smith Would it be helpful to schedule a call to talk through this style? |
|
@agruber reached out and is also interested in helping out here. |
|
This is next up on my list to review. I figured what I'd do is go through this, including with the test library, and flag anything that I think is unreasonable or too fragile (as well, of course, as anything that I think is actually wrong). If there's a lot, jumping on a phone call will probably be most efficient, if there's just a couple of things we can hash them out here for transparency. |
|
OK, I'm about 1/3 through, looking very good so far, I only have a handful of notes. Will send all together once I'm done. |
There was a problem hiding this comment.
OK, I've done a code review and except for a couple of nits and a couple of questions, this looks all great. Very helpful comments, too, so thanks for that.
I did a quick look through the test library to make sure everythign looks good in broad strokes.
Using this as a test case for style-specific test-suites would be great (needn't be prior to accepting this, though -- I think once we settled on the few remaining issues, we can take this).
apa.csl
Outdated
| <text variable="genre"/> | ||
| <text term="issue" form="short" text-case="capitalize-first"/> | ||
| </group> | ||
| <text variable="genre"/> |
There was a problem hiding this comment.
I don't like genre for U.S. Patent. I usually do authority which has the added benefit of being in the Zotero model as issuingAuthority -- I'm not sure we should add "type" to patent.
There was a problem hiding this comment.
I didn't change this at all. APA style wants U.S. patent No. 12345.
I agree that authority would be good for the country—importing from Google Patents puts the full "United States" in issuingAuthority, PATO.gov leaves it empty; we can rely on users to shorten to "US" if desired.
That leaves out the word "patent". We could add change it year to show both authority then either genre (if present) or hard-coded text "patent" (and replace with term="patent" when available), like we do with dataset.
There was a problem hiding this comment.
The reason to include genre here would be to accommodate things like trademark applications, which have the same citation form.
There was a problem hiding this comment.
How about something like this (conceptually):
<text variable="authority"/>
<group delimiter=" ">
<choose>
<if variable="genre">
<text variable="genre"/>
</if>
<else>
<text value="patent"/>
</else>
</choose>
<group delimter=" ">
<text term="issue" form="short"/>
</text variable="number"/>
</group>
</group>
This follows the same logic we do for presentations and personal communication. And then handle the bibliography in a similar way.
- "book report" match="any" - Commenting about logic - Localization notes
|
😃 Your submission passed all our automated tests. |
|
😃 Your submission passed all our automated tests. |
I emailed APA Style and they indicated that this was acceptable.
|
😃 Your submission passed all our automated tests. |
|
I think we have everything covered then, right? Ready to pull the trigger? |
|
I will copy the changes to the other APA styles this afternoon. Then, we can pull the trigger. |
|
😟 There are some issues with your submission. Please check the test report for details. |
Remove unused macros to pass Travis. Also add `annote` as block variable to permit display of common CV annotations (e.g., awards, authorship notes).
|
😟 There are some issues with your submission. Please check the test report for details. |
Remove one more unused macro from apa-cv
|
😃 Your submission passed all our automated tests. |
|
Okay, @adam3smith, let's pull the trigger! |
I've done testing, but I need to do more checking still. Is there a good set of test items available to start with to check that all of the conditionals are working? I can make my own test suite, but if there is a good starting set that will save a lot of time.