Skip to content

/styles

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

APA overhaul #3602

Merged
merged 52 commits into from Mar 25, 2019

Conversation

Reviewers

@adam3smith adam3smith

Assignees

@adam3smith adam3smith

Labels
pinned
Projects
None yet
Milestone
No milestone
3 participants
@bwiernik @csl-bot @adam3smith
@bwiernik
Copy link
Contributor

bwiernik commented Jul 10, 2018

  • Added support for interviews, archival material, and audiovisual materials
  • Updated the code for the (description) and [format] blocks to accommodate the various item cases and ensure that only one of each gets printed for any given reference

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.

bwiernik added some commits Jul 7, 2018

@bwiernik
APA bug fix 
Fixes regression in #3590
Loading status checks…
51e4d91
@bwiernik
Don't show publisher if other access information is available 
- Also some other minor changes in preparation of a unified format macro
1dd5d15
@bwiernik
Allow report to potentially refer to a chapter in a report 
- And show translators, etc. for webpages and blogposts
acd844b
@bwiernik
Update APA (description) [format] container 
To meet all of the various requirements

Needs testing
Need to check the rules for:
- patent
- treaty
- song
- interview
9e9d3be
@bwiernik
Fix typos
c03c8a4
@bwiernik
Update legal_cites, patents, and creators 
- Update legal_cites (bill, legal_case, legislation, treaty)
- Update patents
- Add additional creators alongside "author"
957085b
@bwiernik
Bug fixes and robustification
Loading status checks…
35641e4
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Jul 10, 2018

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.
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Jul 10, 2018

😃 Your submission passed all our automated tests.
@bwiernik

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bwiernik commented Jul 10, 2018

Happy to discuss some of the decisions that had to be made. Specific areas that were challenging were accommodating:

  1. Archival and non-archival manuscripts
  2. Personal communications that are published/available versus not
  3. Reports and songs that have container-titles versus don't

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.
@bwiernik

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bwiernik commented Jul 13, 2018 •
edited

Question:

Is there any way to get a names element to render multiple creator types together, with the order presented in the item data, rather than separated by creator type? For example, if a book has two authors and an illustrator, then this:
<names variable="author illustrator" delimiter=", ">

Produces:
Author, F., & Author, S., Illustrator, F. (Ill.)

Rather than:
Author, F., Author, S., & Illustrator, F. (Ill.)

I had thought this code would produce the latter output (essentially all of the types inside a single variable command as equivalent. But re-reading the specification, it is clear that it doesn’t. This seems like a major limitation.

At minimum, it seems like there should be a way to specify that and should apply to the full list of creators, rather than to each sub-list. Even in the current APA style, it’s noy possible to correctly format a chapter in a book with both an editor and translator (who are separate people).
@adam3smith

This comment has been minimized.

Sign in to view
Copy link
Member

adam3smith commented Jul 13, 2018

No -- every name call (i.e. every creator type) is being handled separately. I have to say that I find the second example really confusing, too, with no way of telling who the (Ill) refers to.
@bwiernik

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bwiernik commented Jul 13, 2018

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:
In A. S. Smith (Ed.), T. Jones (Trans.), Book

Rather than with the ampersand:
In A. S. Smith (Ed.) & T. Jones (Trans.), Book

This CSL behavior seems very counter intuitive to me. Particularly frustrating is that and can only be applied to name and not to names, so it is impossible to create continuous lists with multiple creator types. For example, it’s impossible to correctly format editor and translator combinations or to even approach a correct rendering for media creators:
Allen, A. (Director), Berry, B. (Director), & Collins, C. (Producer)
@bwiernik
Adjust contributor inclusion and formatting 
Including multiple creator types in one names string with consistent delimiter/and/et-al results isn't currently possible in CSL.
Loading status checks…
2182a37
@bwiernik

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bwiernik commented Jul 13, 2018

(But thanks for the clarification!)
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Jul 13, 2018

😃 Your submission passed all our automated tests.
@bwiernik
Bug fix in newspaper
Loading status checks…
02ffda9
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Jul 13, 2018

😃 Your submission passed all our automated tests.
@bwiernik
Fix broadcast container bug
Loading status checks…
04033ed
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Jul 13, 2018

😃 Your submission passed all our automated tests.
@bwiernik
Adjust names subsitution 
`illustrator` currently isn't be rendered at all, probably due to a citeproc-js bug
Loading status checks…
a99710a
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Jul 14, 2018

😃 Your submission passed all our automated tests.
@bwiernik
Update song recording
Loading status checks…
8bf6cdc
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Jul 14, 2018

😃 Your submission passed all our automated tests.
@bwiernik

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bwiernik commented Jul 14, 2018

@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.
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Jul 14, 2018

😃 Your submission passed all our automated tests.
@adam3smith

adam3smith reviewed Aug 4, 2018
View changes

Copy link
Member

adam3smith left a 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.
Show resolved Hide resolved apa.csl Outdated
Show resolved Hide resolved apa.csl Outdated
Show resolved Hide resolved apa.csl Outdated
Show resolved Hide resolved apa.csl Outdated
Show resolved Hide resolved apa.csl Outdated
Show resolved Hide resolved apa.csl Outdated
Show resolved Hide resolved apa.csl Outdated
Show resolved Hide resolved apa.csl Outdated
Show resolved Hide resolved apa.csl Outdated
@adam3smith

This comment has been minimized.

Sign in to view
Copy link
Member

adam3smith commented Aug 4, 2018

Oh, and of course thanks for doing this in the first place. (And so far I've only reviewed the APA changes.
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Feb 20, 2019

😃 Your submission passed all our automated tests.
@bwiernik
Fix preprint publisher
Loading status checks…
d4eae2c
@bwiernik

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bwiernik commented Feb 20, 2019 •
edited

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 incorrect output. Of those, CSL change indicates changes to CSL that are needed to meet the formatting requirements. weird APA notes places where APA is inconsistent examples or rules that are too difficult to be able to implement effectively.

It should be ready for your review now @adam3smith
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Feb 20, 2019

😃 Your submission passed all our automated tests.
@bwiernik

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bwiernik commented Mar 1, 2019

@adam3smith Would it be helpful to schedule a call to talk through this style?
@bwiernik

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bwiernik commented Mar 8, 2019

@agruber reached out and is also interested in helping out here.
@adam3smith

This comment has been minimized.

Sign in to view
Copy link
Member

adam3smith commented Mar 12, 2019

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.
Thanks for doing this!
@adam3smith

This comment has been minimized.

Sign in to view
Copy link
Member

adam3smith commented Mar 17, 2019

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.

@fbennett fbennett referenced this pull request Mar 17, 2019

Open

Style dev assist: tests with watch #3994

@adam3smith

adam3smith reviewed Mar 18, 2019
View changes

Copy link
Member

adam3smith left a 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).
Show resolved Hide resolved apa.csl
apa.csl Outdated
<text variable="genre"/>
<text term="issue" form="short" text-case="capitalize-first"/>
</group>
<text variable="genre"/>

This comment has been minimized.

Sign in to view
Copy link
@adam3smith

adam3smith Mar 18, 2019

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@bwiernik

bwiernik Mar 19, 2019

Author Contributor

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.

This comment has been minimized.

Sign in to view
Copy link
@bwiernik

bwiernik Mar 19, 2019

Author Contributor

The reason to include genre here would be to accommodate things like trademark applications, which have the same citation form.

This comment has been minimized.

Sign in to view
Copy link
@adam3smith

adam3smith Mar 20, 2019

Member

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.
Show resolved Hide resolved apa.csl
Show resolved Hide resolved apa.csl
Show resolved Hide resolved apa.csl Outdated
Show resolved Hide resolved apa.csl
Show resolved Hide resolved apa.csl Outdated
Show resolved Hide resolved apa.csl
Show resolved Hide resolved apa.csl Outdated
@bwiernik
Address comments from adamsmith 
- "book report" match="any"
- Commenting about logic
- Localization notes
Loading status checks…
ef0ffe0
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Mar 19, 2019

😃 Your submission passed all our automated tests.
@bwiernik
Fix patent rendering
Loading status checks…
d300778
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Mar 20, 2019

😃 Your submission passed all our automated tests.
@bwiernik
Let URL/DOI replace publisher for patents 
I emailed APA Style and they indicated that this was acceptable.
Loading status checks…
214d6d7
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Mar 22, 2019

😃 Your submission passed all our automated tests.
@adam3smith

This comment has been minimized.

Sign in to view
Copy link
Member

adam3smith commented Mar 22, 2019

I think we have everything covered then, right? Ready to pull the trigger?
@bwiernik

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bwiernik commented Mar 22, 2019

I will copy the changes to the other APA styles this afternoon. Then, we can pull the trigger.
@bwiernik
Update other APA styles
Loading status checks…
af852ea
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Mar 24, 2019

😟 There are some issues with your submission. Please check the test report for details.
@bwiernik
Update apa-cv.csl 
Remove unused macros to pass Travis. Also add `annote` as block variable to permit display of common CV annotations (e.g., awards, authorship notes).
Loading status checks…
ddb9be4
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Mar 24, 2019

😟 There are some issues with your submission. Please check the test report for details.
@bwiernik
Update apa-cv.csl 
Remove one more unused macro from apa-cv
Loading status checks…
e724187
@csl-bot

This comment has been minimized.

Sign in to view
Copy link

csl-bot commented Mar 24, 2019

😃 Your submission passed all our automated tests.
@bwiernik

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bwiernik commented Mar 24, 2019

Okay, @adam3smith, let's pull the trigger!

@adam3smith adam3smith merged commit 0b94edb into citation-style-language:master Mar 25, 2019
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bwiernik bwiernik deleted the bwiernik:apa-overhaul branch Mar 25, 2019

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.