-
Notifications
You must be signed in to change notification settings - Fork 2
Let find() take format as argument to allow returning as list #10
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
base: main
Are you sure you want to change the base?
Changes from all commits
5a719c6
eb26509
ca7c3a1
5c6b8dc
aec4772
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -32,15 +32,18 @@ def __init__(self, verbose=False): | |||||
| # check service availability | ||||||
| resilient_requests('get', SITE_URL + '/ping') | ||||||
|
|
||||||
| def find(self, query, keyword=None, ids=None, max_matches=MAX_MATCHES, matches_per_page=MATCHES_PER_PAGE): | ||||||
| def find(self, query, keyword=None, ids=None, max_matches=MAX_MATCHES, matches_per_page=MATCHES_PER_PAGE, format=str): | ||||||
| """ | ||||||
| Search function for the hepdata database. Calls hepdata.net search function. | ||||||
|
|
||||||
| :param query: string passed to hepdata.net search function. See advanced search tips at hepdata.net. | ||||||
| :param keyword: filters return dictionary for given keyword. Exact match is first attempted, otherwise partial match is accepted. | ||||||
| :param ids: accepts one of ("arxiv", "inspire", "hepdata"). | ||||||
| :param max_matches: maximum number of matches to return. Default is 10,000. | ||||||
| :param matches_per_page: number of matches per page. Default is 10. | ||||||
| :param format: specifies the return format if 'ids' is specified. Allowed formats are: str, list, set, tuple. Default is str. | ||||||
|
|
||||||
| :return: returns a list of (filtered if 'keyword' is specified) dictionaries for the search matches. If 'ids' is specified it instead returns a list of ids as a string. | ||||||
| :return: returns a list of (filtered if 'keyword' is specified) dictionaries for the search matches. If 'ids' is specified it instead returns a list of ids in the format 'format'. | ||||||
| """ | ||||||
| find_results = [] | ||||||
| for counter in range(int(max_matches / matches_per_page)): | ||||||
|
|
@@ -53,7 +56,7 @@ def find(self, query, keyword=None, ids=None, max_matches=MAX_MATCHES, matches_p | |||||
| # return full list of dictionary | ||||||
| find_results += data['results'] | ||||||
| else: | ||||||
| assert ids in [None, "arxiv", "inspire", "hepdata", "id"], "allowd ids are: arxiv, inspire and hepdata" | ||||||
| assert ids in [None, "arxiv", "inspire", "hepdata", "id"], "allowed ids are: arxiv, inspire and hepdata" | ||||||
| if ids is not None: | ||||||
| if ids == "hepdata": | ||||||
| ids = "id" | ||||||
|
|
@@ -76,7 +79,14 @@ def find(self, query, keyword=None, ids=None, max_matches=MAX_MATCHES, matches_p | |||||
| if ids is None: | ||||||
| return find_results | ||||||
| else: | ||||||
| return ' '.join(find_results) | ||||||
| if format==str: | ||||||
| return ' '.join(find_results) | ||||||
| elif format==list: | ||||||
| return find_results | ||||||
| elif format in (set, tuple): | ||||||
| return format(find_results) | ||||||
| else: | ||||||
| raise TypeError(f"Cannot return results in specfied format: {format}. Allowed formats are: {str}, {list}, {set}, {tuple}.") | ||||||
|
||||||
| raise TypeError(f"Cannot return results in specfied format: {format}. Allowed formats are: {str}, {list}, {set}, {tuple}.") | |
| raise TypeError(f"Cannot return results in specified format: {format}. Allowed formats are: {str}, {list}, {set}, {tuple}.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not covered by the tests, resulting in a -0.4% decrease in test coverage for the PR. Please add a test that raises the TypeError exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in error message: 'specfied' should be 'specified'. This is a user-facing error message that will appear unprofessional with the typo.
Did we get this right? 👍 / 👎 to inform future reviews.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| __version__ = "0.3.0" | ||
| __version__ = "0.3.1" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,16 +4,19 @@ | |
|
|
||
| from click.testing import CliRunner | ||
|
|
||
| from hepdata_cli.api import Client | ||
| from hepdata_cli.api import Client, MAX_MATCHES, MATCHES_PER_PAGE | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why import |
||
| from hepdata_cli.cli import cli | ||
|
|
||
|
|
||
| # arguments for testing | ||
|
|
||
| test_api_find_arguments = [ | ||
| ('reactions:"P P --> LQ LQ X"', None, None), | ||
| ('reactions:"P P --> LQ LQ"', 'year', None), | ||
| ('phrases:"(diffractive AND elastic)"', None, 'arxiv'), | ||
| ('reactions:"P P --> LQ LQ X"', None, None, None), | ||
| ('reactions:"P P --> LQ LQ"', 'year', None, None), | ||
| ('phrases:"(diffractive AND elastic)"', None, 'arxiv', str), | ||
| ('phrases:"(diffractive AND elastic)"', None, 'hepdata', list), | ||
| ('reactions:"P P --> LQ LQ X"', None, 'arxiv', set), | ||
| ('reactions:"P P --> LQ LQ X"', None, 'inspire', tuple), | ||
| ] | ||
|
|
||
| test_cli_find_arguments = [ | ||
|
|
@@ -24,17 +27,16 @@ | |
|
|
||
| # api test | ||
|
|
||
| @pytest.mark.parametrize("query, keyword, ids", test_api_find_arguments) | ||
| def test_api_find(query, keyword, ids): | ||
| @pytest.mark.parametrize("query, keyword, ids, format", test_api_find_arguments) | ||
| def test_api_find(query, keyword, ids, format): | ||
| client = Client(verbose=True) | ||
| search_result = client.find(query, keyword, ids) | ||
| search_result = client.find(query, keyword, ids, format=format) | ||
| if ids is None: | ||
| assert type(search_result) is list | ||
| if len(search_result) > 0: | ||
| assert all([type(entry) is dict for entry in search_result]) | ||
| else: | ||
| assert type(search_result) is str | ||
|
|
||
| assert type(search_result) is format | ||
|
|
||
| # cli testing | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing spaces around the equality operator. Should be
if format == str:to follow PEP 8 style guidelines.