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

Fix locator conditions (resolve #142) #143

Merged
merged 3 commits into from
Dec 10, 2024

Conversation

zepinglee
Copy link
Contributor

@zepinglee zepinglee commented Jul 15, 2024

This patch fixes issue #142.

In the original code, item.locator.label raises VariableError since there is no locator in the CitationItem. This error is then ignored in process_children() called in Layout.render_citation(), which causes empty output.

citeproc-py/citeproc/model.py

Lines 1470 to 1472 in d7fb0a0

def _locator(self, item):
return [var.replace('-', ' ') == item.locator.label
for var in self.get('locator').split()]

try:
text = child.process(item, **kwargs)
if text is not None:
output.append(text)
except VariableError:
pass

@yarikoptic
Copy link
Collaborator

could a test be added which would catch original bug and now pass?

@zepinglee
Copy link
Contributor Author

could a test be added which would catch original bug and now pass?

Is there any directory holding similars tests?

I suggest adding a local directory similar to test-suite/processor-tests/humans with tests in the same format (e.g., affix_CommaAfterQuote.txt). This requires modifying tests/citeproc-test.py.

@zepinglee
Copy link
Contributor Author

zepinglee commented Jul 26, 2024

@yarikoptic In 8ae7c71, I added a directory for local tests in the format of the test-suite.

In the master branch, the bug is revealed as follows.

$ PYTHONPATH=. py tests/citeproc-test.py condition_LocatorVariables -v
>>> Testing condition_LocatorVariables.txt
EXP: (Doe)
     (Doe 23)
RES: ()
     (Doe 23)
<<< FAILED

My patch fixes the bug.

$  PYTHONPATH=. py tests/citeproc-test.py condition_LocatorVariables -v
>>> Testing condition_LocatorVariables.txt
EXP: (Doe)
     (Doe 23)
RES: (Doe)
     (Doe 23)
<<< SUCCESS

Copy link
Contributor

@tmorrell tmorrell left a comment

Choose a reason for hiding this comment

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

Thanks @zepinglee. The change looks great and for adding the local test structure. Apologies for the delay in getting this looked at.

@tmorrell
Copy link
Contributor

@zepinglee Could you rebase this PR so the CI can run? I tried to do it for you but don't seem to have permission.

@@ -22,6 +22,7 @@
'test-suite'))
TEST_PARSER_PATH = os.path.join(CITEPROC_TEST_PATH, 'processor.py')
TESTS_PATH = os.path.join(CITEPROC_TEST_PATH, 'processor-tests', 'humans')
LOCAL_TESTS_PATH = os.path.join(os.path.dirname(__file__), "local")
Copy link
Collaborator

Choose a reason for hiding this comment

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

what meaning did you imply by "local"? should it be may be "resources" or "files"?

in any case -- would also need to tune up MANIFEST.in to include .txt files I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what meaning did you imply by "local"? should it be may be "resources" or "files"?

It means local tests in this repository rather than tests from the standard test-suite. It's named following the fixtures/local directory of citeproc-js.

in any case -- would also need to tune up MANIFEST.in to include .txt files I guess.

I've added 7ade76c.

@zepinglee
Copy link
Contributor Author

@zepinglee Could you rebase this PR so the CI can run? I tried to do it for you but don't seem to have permission.

Done.

MANIFEST.in Outdated
@@ -4,3 +4,4 @@ recursive-include docs *.html *.css *.png
recursive-include examples *.py *.csl *.bib
recursive-include tests *.py *.bib
prune tests/citeproc-test
prune tests/local
Copy link
Collaborator

Choose a reason for hiding this comment

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

it should be included, not pruned per se. tests/citeproc-test seems to be some clone which tests do "behind the curtain". I think you should instead add *.txt into line above

recursive-include tests *.py *.bib *.txt

and please check if it does include those files in source distribution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should be included, not pruned per se. tests/citeproc-test seems to be some clone which tests do "behind the curtain". I think you should instead add *.txt into line above

I've modified in dbbb5de.

and please check if it does include those files in source distribution

Yes, the tests/local/*.txt files are included.

@yarikoptic yarikoptic merged commit 42cda9b into citeproc-py:master Dec 10, 2024
17 checks passed
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.

4 participants