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

Fixed some errors in translator for "ABC News Australia" #3222

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

franklindyer
Copy link
Contributor

Contributed the following to ABC News Australia.js:

  1. Made contentType comparison for detection case-insensitive. Case-sensitivity was causing one of the test cases to fail because the contentType was video rather than Video.
  2. Added code to detect authors by grabbing their names from elements with a data-uri attribute starting with coremedia://person/, when such elements exist. Previous method of author name detection is now used as a fallback. The previous method fails when author titles such as "weather correspondent" or "global affairs editor" are present.
  3. Added a third test case to reflect the above quirk.

Before these modifications, the first test case failed and the second failed detection. Now the first test case passes and the second passes detection, but still fails due to an incorrect abstractNote. But this appears to be an issue with a different translator that is being used here (whichever one has GUID equal to 951c027d-74ac-47d4-a107-9c3069ab7b48).

P.S. This is my first contribution, just learning how Zotero translators work! :-)

@franklindyer
Copy link
Contributor Author

Hi! For some reason, my pull request is always failing at the Test pull request stage, even though all of my test cases succeed on my end. But I'm noticing that several of the most recently merged pull requests also fail at this stage.

The error message is kind of opaque. Is it a problem that my PR is failing here?

@aborel
Copy link
Contributor

aborel commented Jan 10, 2024

The tests have been broken for a while, work to fix them is in progress in PR #3210 .

@@ -38,13 +38,14 @@

function detectWeb(doc, _url) {
let contentType = attr(doc, 'meta[property="ABC.ContentType"]', 'content');
if (contentType == 'CMChannel' && getSearchResults(doc, true)) {
contentType = contentType.toUpperCase(); // Case-insensitive treatment of content type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: one space before a comment in new code.

Comment on lines 117 to 118
for (let i = 0; i < authorNameLinks.length; i++) {
let authorName = authorNameLinks[i].innerText;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (let i = 0; i < authorNameLinks.length; i++) {
let authorName = authorNameLinks[i].innerText;
for (let authorNameLink of authorNameLinks) {
let authorName = authorNameLink.textContent;

Prefer for..of over indexed for on arrays; textContent instead of innerText for reasons of convention and because innerText includes presentation (e.g. uppercase conversion that's specified in CSS) and textContent doesn't.

Comment on lines 129 to 130
for (let i = 0; i < authorsList.length; i++) {
item.creators.push(ZU.cleanAuthor(authorsList[i], "author"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (let i = 0; i < authorsList.length; i++) {
item.creators.push(ZU.cleanAuthor(authorsList[i], "author"));
for (let author of authorsList) {
item.creators.push(ZU.cleanAuthor(author, "author"));

@AbeJellinek
Copy link
Member

Tests might work if you rebase.

- fixed comment styling
- prefer `let x of xs` sugar to indexed `for` loops
- prefer `textContent` to `innerText`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants