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

Reason for citation order in bugreports_ChicagoAuthorDateLooping? #66

Open
Jason-Abbott opened this issue Jun 22, 2023 · 10 comments
Open

Comments

@Jason-Abbott
Copy link
Contributor

This is a citation test that expects

(Anon.; Anon.; Manstein 1982)

However, Manstein is first in the INPUT section and <citation/> has no <sort/> to change that so I render it as

(Manstein 1982; Anon.; Anon.) 

With tests like this that don’t have an explicit CITATION-ITEMS or CITATIONS section, I’ve always taken the INPUT order to be the citation order, then applied the specification, “In the absence of cs:sort, cites and bibliographic entries appear in the order in which they are cited.”

The test does have a bibliography sort that creates the expected order but I don’t know why that would affect citation order in this case.

What am I missing?

@adam3smith
Copy link
Member

You're not missing anything -- not sure what's going on in the test fixture, which seems to be about disambiguation rather than sort order.
In a quick test, current Zotero/citeproc-js also renders Manstein first if it's the first citation and that's clearly the right behavior.
This might suggest that the text fixture doesn't go by the INPUT order, but that'd be odd and I have the same interpretation of the INPUT order as you.

@Jason-Abbott
Copy link
Contributor Author

Jason-Abbott commented Jun 22, 2023

If current Zotero/citeproc-js renders Manstein first then any objection if I submit a PR to change the test accordingly? As you note, the point of the test isn't sorting so I think it's worth having it runnable for those other reasons.

@adam3smith
Copy link
Member

@fbennett -- is that test passing for citeproc-js in your tests currently? If so, any idea why?

Jason-Abbott added a commit to toba/csl-test-suite that referenced this issue Jun 22, 2023
@zepinglee
Copy link
Contributor

With tests like this that don’t have an explicit CITATION-ITEMS or CITATIONS section, I’ve always taken the INPUT order to be the citation order, then applied the specification, “In the absence of cs:sort, cites and bibliographic entries appear in the order in which they are cited.”

The test does have a bibliography sort that creates the expected order but I don’t know why that would affect citation order in this case.

What am I missing?

I was stuck in the same problem until I read the code of citeproc-test-runner. The citation order is taken from the registry after updateItems() which preforms the bibliography sorting.

https://github.com/Juris-M/citeproc-test-runner/blob/b1e72d5cb1363b7f4abbe1f6546c9e2c443db726/lib/sys.js#L369-L376

@fbennett
Copy link
Member

fbennett commented Jun 22, 2023 via email

@adam3smith
Copy link
Member

Thanks @zepinglee that makes sense.

So yes to this

Haven't looked at the code, but that sounds right. It would mean that the test result should be amended, and the (current) citeproc-js test runner should fail?

as well as to this

If current Zotero/citeproc-js renders Manstein first then any objection if I submit a PR to change the test accordingly?

@zepinglee
Copy link
Contributor

The fixture disambiguate_InitializeWithButNoDisambiguation.txt is also involved in this behavior. If the citation order is directly taken from the INPUT, the result will be (Doe, 1965b, 1965a) rather than (Doe, 1965a, 1965b).

@fbennett
Copy link
Member

fbennett commented Jun 23, 2023 via email

@Jason-Abbott
Copy link
Contributor Author

Jason-Abbott commented Jun 23, 2023

@zepinglee, I understand from your comment that a test's citation order does, in some cases, follow the bibliography order. So a PR to change these tests (including the other test you note) will cause, as @fbennett mentions, the current citeproc-js test runner to fail.

The alternative, which wouldn't take too much code now that I see the issue more clearly, would be to apply these rules:

  • If a test does not have an explicit CITATION-ITEMS or CITATIONS section then citations should be derived from INPUT after bibliography sorting
  • If a test does have explicit citations then their order will not be (directly) affected by bibliography sorting

This isn't my favorite alternative (will muddy up my code a bit) but I hesitate to suggest a PR that breaks long-standing existing code.

@zepinglee
Copy link
Contributor

The alternative, which wouldn't take too much code now that I see the issue more clearly, would be to apply these rules:

  • If a test does not have an explicit CITATION-ITEMS or CITATIONS section then citations should be derived from INPUT after bibliography sorting
  • If a test does have explicit citations then their order will not be (directly) affected by bibliography sorting

Yes, that's correct.

Also note that current citeproc-test-runner performs updateItems() if there is no CITATIONS section. This function not only sorts the bibliography but also disambiguates names and it should affect several of the tests that have only CITATION-ITEMS sections.

This isn't my favorite alternative (will muddy up my code a bit) but I hesitate to suggest a PR that breaks long-standing existing code.

I agree. It's a better solution if @fbennett would like to synchronously modify citeproc-test-runner and the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants