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.

Sign up for GitHub

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

Jump to bottom

Issue40-integrate-pagination #43

Merged
merged 7 commits into from
Feb 13, 2024
Merged

Issue40-integrate-pagination #43

merged 7 commits into from
Feb 13, 2024

Conversation

k-allagbe
Copy link
Member

@k-allagbe k-allagbe commented Feb 12, 2024 •
edited
Loading

Closes #40: integrate pagination

  • updated azure route and search method call
  • refactored flask config to be a dict instead of an object
  • updated envs in .env.template
  • updated tests
  • updated docs
  • point azure-db dependency to the main branch

@k-allagbe
issue #40: update azure search function call
feed4d4
@k-allagbe
issue #40: update route
36a73b5
@k-allagbe
issue #40: refactor flask config + update envs
91c3485 
Having a dataclass manage the flask config by using default values gets clunky, especially for testing. Dicts are easier to work with. I used TypedDict only for type hinting.
@k-allagbe
issue #40: update tests
2a750e1
@k-allagbe
issue #40: update docs
53efefe
@k-allagbe k-allagbe mentioned this pull request Feb 12, 2024
Closed
6 tasks
ibrahim-kabir
ibrahim-kabir reviewed Feb 13, 2024
View reviewed changes
app/config.py Outdated Show resolved Hide resolved
@k-allagbe k-allagbe force-pushed the issue40-integrate-pagination branch from a87fe4f to ca67a7e Compare February 13, 2024 17:09
@ibrahim-kabir ibrahim-kabir self-requested a review February 13, 2024 18:16
ibrahim-kabir
ibrahim-kabir approved these changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@ibrahim-kabir ibrahim-kabir left a comment

Choose a reason for hiding this comment

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

LGTM

rngadam
rngadam approved these changes Feb 13, 2024
View reviewed changes
Copy link

@rngadam rngadam left a comment

Choose a reason for hiding this comment

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

I'm seeing a lot of leakage between Azure configuration settings into finesse-backend to the point it seems hard to separate both. Guess we'll see into refactoring this when we try to integrate LlamaIndex

@k-allagbe k-allagbe force-pushed the issue40-integrate-pagination branch from 50afd35 to 73df7a4 Compare February 13, 2024 20:54
@k-allagbe k-allagbe merged commit 730c319 into main Feb 13, 2024
7 checks passed
@k-allagbe k-allagbe mentioned this pull request Mar 5, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibrahim-kabir ibrahim-kabir ibrahim-kabir approved these changes

@rngadam rngadam rngadam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate Pagination from azure-db into finesse-backend
3 participants
@k-allagbe @rngadam @ibrahim-kabir