-
Notifications
You must be signed in to change notification settings - Fork 2
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
Added endpoint to return lei data from list of leis #20
Changes from 4 commits
19afd1e
c5eacb9
380c21a
3256034
87ffe31
9abeb0c
590c933
2b3f7ae
b368c95
e7675de
7f98238
26e5e0e
69e77a8
6f9c391
746d666
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. I think we should rename this to 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. actually have a 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.
The naming convention was due to the request parameter naming.
Completely agree with this. @hkeeler would there be issue with leaving it this way for now? 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. OK by me. I mean, I guess it depends on which PR gets merged first. 😉 If #21 get merged, then you should move this into that new file here. If PR goes first, then @lchen-2101 should incorporate it. Tick tock! ⏱️ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
from typing import List, Optional | ||
|
||
from fastapi import Query | ||
|
||
|
||
def parse_leis(leis: List[str] = Query(None)) -> Optional[List]: | ||
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. since we're using 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. This doesn't seem LEI-specific. I'm guessing we'll have additional multi-value query parameters. Perhaps this could be Also, there's some interesting discussion on this topic: fastapi/fastapi#8225 I like the one that creates a custom 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. Also, the return type is 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. about making it generic, I should've left the comment that I deleted, it's because the name of the parameter being |
||
""" | ||
Parses leis from list of one string to list of multiple distinct lei strings | ||
Returns empty list when nothing is passed in | ||
Ex: ['lei1,lei2'] -> ['lei1', 'lei2'] | ||
""" | ||
|
||
if leis is None or len(leis) == 0 or leis[0] == "": | ||
return [] | ||
else: | ||
return leis[0].split(',') |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,14 @@ async def test_get_institutions_by_domain_not_existing(self, session: AsyncSessi | |
res = await repo.get_institutions(session, domain="testing.bank") | ||
assert len(res) == 0 | ||
|
||
async def test_get_institutions_by_lei_list(self, session: AsyncSession): | ||
res = await repo.get_institutions(session, leis=['TESTBANK123']) | ||
assert len(res) == 1 | ||
|
||
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. We should also test scenarios with multiple LEIs. Probably want scenarios where all requested values are present, and where only some are present. Looks like that'll require adding at least one more FI instance to the above fixture. |
||
async def test_get_institutions_by_lei_list_item_not_existing(self, session: AsyncSession): | ||
res = await repo.get_institutions(session, leis=["NOTTESTBANK"]) | ||
assert len(res) == 0 | ||
|
||
async def test_add_institution(self, session: AsyncSession): | ||
await repo.upsert_institution( | ||
session, FinancialInstitutionDao(name="New Bank 123", lei="NEWBANK123") | ||
|
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.
let's order the args here to match the api endpoint, better readability on the precedence.