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

page_Expand test sometimes updates page range delimiter, sometimes not #54

Closed
Jason-Abbott opened this issue Apr 23, 2023 · 6 comments
Closed

Comments

@Jason-Abbott
Copy link
Contributor

The expected output for page_Expand sometimes retains the original page range delimiter character (retaining a dash) and sometimes it replaces it with the locale standard (converting dash to en-dash).

So always keeping the existing delimiter doesn’t work and always replacing it doesn’t work. I can’t discern the expected logic.

It seems like the range character should always be updated to the standard when re-formatting a range. What am I missing?

Example: minimal, at 110–115
Example: prefix on first number only, at N110-5
Example: same prefix on both numbers, at N110–N115
Example: prefix on last number only, at 110-N6
Example: different prefixes on both numbers, at N110-P5
Example: leading number before prefix, at 123N110-N5
Example: multiple ranges, at 123N110-N5, 456K200-99
Example: first less than second, at 123N110-N5, 000c23-22
@Jason-Abbott
Copy link
Contributor Author

This seems to be pertinent documentation:

The “locator” variable is always rendered with an en-dash replacing any hyphens. For the “page” variable, this replacement is only performed if the page-range-format attribute is set on cs:style

In this case, the page-range-format attribute is set so it seems, contrary to the test, that all dashes should be en-dashes.

@adam3smith
Copy link
Member

adam3smith commented May 4, 2023

I'm pretty sure the rationale here is that not all hyphens indicate ranges -- they can also, e.g., be part of an article number, in which case flipping them is clearly wrong. So the test assumes that if you have just numbers or you have numbers with the same prefix, that's a range, otherwise it's not. The relevant commit changing the test (which did use to be all en-dashes) also suggests that rationale.

@Jason-Abbott
Copy link
Contributor Author

Okay, that makes sense. I’ll close this issue. Perhaps confusing is that some of the unchanged dashes are called “ranges” in the test. I guess that’s just how they began, prior to the commit you linked.

@Jason-Abbott
Copy link
Contributor Author

P.S. I’ll be sure to check commit history on tests before opening issues from now on. 👍

@adam3smith
Copy link
Member

I think we'd also take PR's with comments on tests you find confusing. Ideally it shouldn't need a 7 year-old memory of a discussion on the Zotero forums to figure out the rationale for a test ;)

@bdarcus
Copy link
Member

bdarcus commented May 4, 2023

Yes, see #51

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

3 participants