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

feat: implement feeds endpoint #79

Merged
merged 9 commits into from
Aug 20, 2023
Merged

feat: implement feeds endpoint #79

merged 9 commits into from
Aug 20, 2023

Conversation

aronza
Copy link
Contributor

@aronza aronza commented Aug 13, 2023

Summary:

Implements:

Expected behavior:

Screenshots from Postman requests against local API:

Screenshot 2023-08-13 at 3 23 47 PM Screenshot 2023-08-13 at 3 23 00 PM a7"> Screenshot 2023-08-13 at 3 24 07 PM

@aronza aronza requested a review from indraneel August 13, 2023 19:25
@aronza aronza self-assigned this Aug 13, 2023
@welcome
Copy link

welcome bot commented Aug 13, 2023

Thanks for opening this pull request! You're awesome. We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of titles with semantic prefixes:

  • fix: Bug with ssl network connections + Java module permissions.
  • feat: Initial support for multiple @PrimaryKey annotations.
  • docs: update RELEASE.md with new process
    To get this PR to the finish line, please do the following:
  • Include tests when adding/changing behavior
  • Include screenshots

@aronza aronza changed the title Implement feeds endpoint feat: implement feeds endpoint Aug 13, 2023
@emmambd emmambd requested a review from jcpitre August 14, 2023 13:50
@davidgamez davidgamez requested a review from cka-y August 15, 2023 17:36
@jcpitre
Copy link
Contributor

jcpitre commented Aug 15, 2023

There are also some query parameters to implement, namely limit and offset.
filter and sort have been delegated to #63
I suggest either you mention clearly in this PR and in the issue that limit and offset are not included, and not close the issue after the PR is closed. or you create a new issue to cover these parameters.

@@ -12,6 +16,24 @@
from feeds_gen.models.latest_dataset import LatestDataset
from feeds_gen.models.source_info import SourceInfo

from database.database import Database

db = Database()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice to have: we should probably set up a mechanism to have a single instance of the database (singleton pattern). Instantiating the database as a global variable might increase the risk of concurrency issues and creates a lack of control over initialization in case we need to make any changes to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to a global variable in database module

db = Database()


def map_feed(feed: Feed):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: this function could be added to a utils file/module if you think it's going to be used by other classes or added as a static method of the FeedApiImpl class otherwise to keep the coding style consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to a static method in this class as this looks specific to this module

@aronza
Copy link
Contributor Author

aronza commented Aug 16, 2023

There are also some query parameters to implement, namely limit and offset. filter and sort have been delegated to #63 I suggest either you mention clearly in this PR and in the issue that limit and offset are not included, and not close the issue after the PR is closed. or you create a new issue to cover these parameters.

Added limit and offset to this PR. filter and sort can be tracked separately as these apply to most endpoints

@aronza
Copy link
Contributor Author

aronza commented Aug 16, 2023

@cka-y we saw that you added changes to provider field in feat_15.sql. The OpenAPI spec still shows providers as a array of strings on BasicFeed. I've changed it to provider as a string

@aronza aronza merged commit c761c16 into main Aug 20, 2023
1 check passed
@aronza aronza deleted the aronza/get-feeds branch August 20, 2023 15:40
@welcome
Copy link

welcome bot commented Aug 20, 2023

🥳 Congrats on getting your first pull request merged!

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.

Develop metadata endpoint Develop feeds/{id} endpoint Develop feeds endpoint
3 participants