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

american-chemical-society Misc. changes #1588

Merged
merged 3 commits into from Jun 18, 2015

Conversation

serpodrick
Copy link
Contributor

Hi

Three changes for the ACS styles as specified in the commit messages.

rmzelle added a commit that referenced this pull request Jun 18, 2015
american-chemical-society Misc. changes
@rmzelle rmzelle merged commit 5b590af into citation-style-language:master Jun 18, 2015
@rmzelle
Copy link
Member

rmzelle commented Jun 18, 2015

Thanks!

@serpodrick serpodrick deleted the acs branch June 18, 2015 14:38
@adam3smith
Copy link
Member

Hey @serpodrick -- was there a particular issue you were addressing with this? It seems like it may be causing problems for Papers. I think that's mainly their implementation issue, but I'm also not entirely clear what scenario you had in mind with this change. E.g. in Mendeley, article is a fallback category, so just using the style's fallback (i.e. else) formatting makes sense to me?

@rmzelle
Copy link
Member

rmzelle commented Sep 18, 2015

Specifically, the problem is that apparently, all "article-journal" items match against "article" in Papers, which creates incorrect output.

@serpodrick
Copy link
Contributor Author

I am translating contribution types from an in-house database and I was trying to account for articles that aren't in a journal, a magazine or a newspaper. This particular change was intended to account for "Newsletter Articles", but it's not the only one.

I don't think citeproc-js uses article when the type is article-journal.

@rmzelle
Copy link
Member

rmzelle commented Sep 18, 2015

I don't think citeproc-js uses article when the type is article-journal.

No, it doesn't, and CSL processors shouldn't. It would be nice if we could fix this in the style, though, at least as an interim option.

One solution would probably be to move up <else-if type="article-journal review" match="any"> as the first entry of <choose/>. That way any "article-journal" item would first match that.

@serpodrick serpodrick restored the acs branch September 18, 2015 14:52
@serpodrick
Copy link
Contributor Author

I pushed a change to do that: e297ab1. Other than validating the files, I didn't test them. (Since this request is merged, it looks like I would need to create a new request, right?)

@rmzelle
Copy link
Member

rmzelle commented Sep 18, 2015

@adam3smith, should we go with the solution from e297ab1 ?

@adam3smith
Copy link
Member

Away from computer, but going by descriptions, yes, sound good

Sent from my phone
On Sep 18, 2015 11:39 AM, "Rintze M. Zelle" notifications@github.com
wrote:

@adam3smith https://github.com/adam3smith, should we go with the
solution from e297ab1
e297ab1
?


Reply to this email directly or view it on GitHub
#1588 (comment)
.

@rmzelle
Copy link
Member

rmzelle commented Sep 18, 2015

@serpodrick, yes, we'd appreciate a fresh PR.

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

3 participants