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 load scopus csv authors affiliations #59

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

Conversation

tleedepriest
Copy link
Contributor

  • added conftest file that supports simple tests on all docs in doc set
  • made scopus_csv more consistent with scopus source (returning None as opposed to empty list and vice versa)
  • add support for authors with multiple affiliations
  • add support for current CSV file download and older version
  • add tests for both versions and break tests out into independent tests.

@stijnh
Copy link
Member

stijnh commented Aug 31, 2023

Hi Travis. Thank you very much this contribution. Fixing the ability to load Scopus CSV files is a highly requested feature. I am happy to see that this PR is well documented and has thorough testing.

I have made some (small) comments on the code.

@tleedepriest
Copy link
Contributor Author

Hi Travis. Thank you very much this contribution. Fixing the ability to load Scopus CSV files is a highly requested feature. I am happy to see that this PR is well documented and has thorough testing.

I have made some (small) comments on the code.

Not sure how to look at the comments? Are you sure they are there or am I missing something?

auths_id = auths_id[:-1]

auths_ids = auths_id.split(";")
auths_ids = [auth_id.lstrip().rstrip() for auth_id in auths_ids]
Copy link
Member

Choose a reason for hiding this comment

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

.lstrip().rstrip() can just become .strip()

"""
auths_id = self.entry.get("Author(s) ID")

if auths_id == "":
Copy link
Member

Choose a reason for hiding this comment

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

This can just become if auths_ids:. If, in the future, the scopus format changes again then self.entry.get("Author(s) ID") could return None. The if auths_ids: will catch both empty strings "" and Nones.


def pytest_generate_tests(metafunc):
path = os.path.dirname(__file__) + "/resources/scopus.csv"
docs = load_scopus_csv(path)
Copy link
Member

@stijnh stijnh Aug 31, 2023

Choose a reason for hiding this comment

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

Will this call load_scopus_csv for ever single test, even those that have nothing to do with scopus? Would it be possible to move it to inside the if statement?

path = os.path.dirname(__file__) + "/resources/scopus.csv"
docs = load_scopus_csv(path)
if "doc" in metafunc.fixturenames:
metafunc.parametrize("doc", docs)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to rename this from doc to scopus_doc? It the future, such a mechanism would be use to also use for all tests of the other data source backends.

Also, should it not be parametrize("doc", docs[0])?

@stijnh
Copy link
Member

stijnh commented Sep 5, 2023

Hi.

You were right. They should be visible now!

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.

2 participants