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

Update Comparative Population Studies #2434

Merged
merged 1 commit into from
Jan 16, 2017
Merged

Update Comparative Population Studies #2434

merged 1 commit into from
Jan 16, 2017

Conversation

bwiernik
Copy link
Member

Remove reference to nonexistent term.

(Perhaps the validator could check for these?)

Remove reference to nonexistent term.

(Perhaps the validator could check for these?)
@csl-bot
Copy link

csl-bot commented Jan 16, 2017

Awesome! You've created a pull request to the Citation Styles Language styles repository. We'll get in touch soon (usually within a day or two). In the meantime, our automated test system will go ahead and run some checks on your pull request. In a few minutes you'll be notified of the test results.

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

To update the current 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.

image

If you need assistance at any point, please leave a comment and we'll get back to you (feel free to write in Dutch, English, French, German, Portuguese, or Spanish).

@csl-bot
Copy link

csl-bot commented Jan 16, 2017

😃 Your submission passed all our automated tests.

@rmzelle
Copy link
Member

rmzelle commented Jan 16, 2017

Remove reference to nonexistent term.

(Perhaps the validator could check for these?)

Well, it's actually a valid term according to the schema (see https://github.com/citation-style-language/schema/blob/v1.0.1/csl-terms.rnc#L130), but it looks like we may have missed listing this type of terms in the specification.

@rmzelle
Copy link
Member

rmzelle commented Jan 16, 2017

(I also think the CSL locale files contain a subset of all allowed terms)

@bwiernik
Copy link
Member Author

Hmm, when I try to insert a report with the style as first submitted in Word, I get a "trmtext not found" error. Perhaps it is a bug with citeproc-js then? In any event, the term line should be removed (there should be no label for number; compare the parallel code on line 196).

@rmzelle
Copy link
Member

rmzelle commented Jan 16, 2017

Perhaps it is a bug with citeproc-js then?

Well, it looks more like a helpful warning that you're trying to use a term for which no translations are available in either the locale files or the style itself.

@rmzelle rmzelle merged commit 1c82a9e into citation-style-language:master Jan 16, 2017
@bwiernik
Copy link
Member Author

"Helpful warning" -- it stops the document from updating at all and prevents the insertion of the citation.

@rmzelle
Copy link
Member

rmzelle commented Jan 16, 2017

If it's that bad, it might be worthwhile reporting the issue to @fbennett. Frank, citeproc-js apparently really doesn't like cases where it can't find a translation for a term. See above. Could it be changed to fail more gently?

@fbennett
Copy link
Member

If the style is missing a term, it does seem like it would be better for the error to be called to the user's attention somehow. The processor can throw a more meaningful error (I'll set that up), but making that easily discoverable would be a job for the calling application. About all the processor could do to surface it would be to throw an ugly citation, which could have bad consequences for authors.

Is it possible to catch these in validation?

@rmzelle
Copy link
Member

rmzelle commented Jan 17, 2017

Is it possible to catch these in validation?

It's tricky, since we currently don't require that each term (or form) is defined in the locale files. I would just default to "" for undefined terms.

@bwiernik
Copy link
Member Author

Could it default to NULL (or similar) so that affixes and group delimiters are not printed around the "" ?

@rmzelle
Copy link
Member

rmzelle commented Jan 17, 2017

Yeah, that probably would be better.

(I'm not totally sure why this bug occurs, though, since we previously already deleted the "author" terms, and we never hear any problems about those: citation-style-language/locales@3b6a1b5)

@fbennett
Copy link
Member

fbennett commented Jan 17, 2017 via email

@rmzelle
Copy link
Member

rmzelle commented Jan 17, 2017

I think that would be preferable, yes. @adam3smith?

@adam3smith
Copy link
Member

yes, I'd rather have this failing silently

@fbennett
Copy link
Member

fbennett commented Jan 19, 2017

I've fixed it for cs:text w/term attribute, and pushed new versions of the Propachi plugins. I think cs:label is already tolerant, but if there a more snags, give a shout. @rmzelle @adam3smith @bwiernik

@rmzelle
Copy link
Member

rmzelle commented Jan 19, 2017

@bwiernik, would you mind testing this with the original style?

@rmzelle
Copy link
Member

rmzelle commented Jan 19, 2017

Also, thanks Frank!

@bwiernik
Copy link
Member Author

cs:text with term="number" still fails with the same error. cs:label with term="number" returns no error.

@bwiernik bwiernik deleted the update-CPoS branch January 19, 2017 10:40
@fbennett
Copy link
Member

fbennett commented Jan 19, 2017 via email

@fbennett
Copy link
Member

I've gotten it to load without error in Zotero 5.0 beta standalone, and the processor replacement seems to be happening. I'll smooth out the install code and do some more testing tomorrow. Looks like we're close though.

@fbennett
Copy link
Member

fbennett commented Jan 20, 2017

Ookay, I think it may be better now. There was a problem with acquiring Zotero from context at startup. Pretty simple, once I figured it out. When the plugin is present at startup, we tie the processor replacement to a final-ui-startup event, which works nicely to assure Zotero is available. When the plugin is installed in the running app, that event has already flown, so we need to install immediately.

The problem was that immediate acquisition crashes everything at startup (ouch -- things blew up even inside a try/catch); and when I staged back to using the event, the new processor was never installed (hence the error on the term refused to go away). I think I've hit some simple incantations to handle both cases -- should be able to enable and disable the plugin and observe the results (remembering that you need to selected another style and select back to observe the change).

Fingers crossed.

@bwiernik
Copy link
Member Author

bwiernik commented Jan 20, 2017

Seems to be working now.

But small citeproc-js bug, I think: Words in "quotes" are not being capitalized when set using Title Case. Example JSON:

[
	{
		"id": "http://zotero.org/users/1386342/items/C8XP258M",
		"type": "article-journal",
		"title": "From \"distance\" to \"friction\": substituting metaphors and redirecting intercultural research",
		"container-title": "Academy of Management Review",
		"page": "905-923",
		"volume": "33",
		"issue": "4",
		"DOI": "10.5465/AMR.2008.34421999",
		"shortTitle": "From \"distance\" to \"friction\"",
		"journalAbbreviation": "AMR",
		"language": "en",
		"author": [
			{
				"family": "Shenkar",
				"given": "Oded"
			},
			{
				"family": "Luo",
				"given": "Yadong"
			},
			{
				"family": "Yeheskel",
				"given": "Orly"
			}
		],
		"issued": {
			"date-parts": [
				[
					"2008"
				]
			]
		}
	}
]

@fbennett
Copy link
Member

fbennett commented Jan 20, 2017 via email

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

Successfully merging this pull request may close these issues.

None yet

5 participants