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

[ENH] Added root endpoint with welcome message and API docs link #286

Merged
merged 5 commits into from
Mar 21, 2024
Merged

[ENH] Added root endpoint with welcome message and API docs link #286

merged 5 commits into from
Mar 21, 2024

Conversation

samadpls
Copy link
Contributor

@samadpls samadpls commented Mar 8, 2024

Changes proposed in this pull request:

  • Created root endpoint with welcome message and API docs link

Checklist

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see https://neurobagel.org/contributing/pull_requests for more info)
  • PR has a label for the release changelog or skip-release (to be applied by maintainers only)
  • PR links to GitHub issue with mention Closes #XXXX
  • Tests pass
  • Checks pass

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

@coveralls
Copy link
Collaborator

coveralls commented Mar 8, 2024

Pull Request Test Coverage Report for Build 8371746917

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 99.788%

Totals Coverage Status
Change from base Build 8193778627: 0.003%
Covered Lines: 940
Relevant Lines: 942

💛 - Coveralls

Signed-off-by: samadpls <[email protected]>
@samadpls
Copy link
Contributor Author

output:
image

@surchs surchs added _community [BOT ONLY] PR label for community contributions. Used for tracking pr-minor Non-breaking feature or enhancement, will increment minor version (0.+1.0) labels Mar 20, 2024
@surchs surchs self-requested a review March 20, 2024 14:20
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Thanks @samadpls for taking this on, this looks good to me. I'd wait for @alyssadai to take a look at my question, but after that I think we'd be good to merge 🧑‍🍳

@alyssadai alyssadai self-requested a review March 20, 2024 16:52
Copy link
Contributor

@alyssadai alyssadai left a comment

Choose a reason for hiding this comment

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

Hey @samadpls, thanks again for addressing this! The welcome message looks great and the link seems to work as expected.

Do you want to take a stab at adding a basic test for the new root endpoint response so that our code coverage check passes? https://github.com/neurobagel/api/blob/main/tests/test_app_events.py might be a good place for it for now, and you can use something like

@pytest.mark.parametrize(
"invalid_data_element_URI",
["apple", "some_thing:cool"],
)
def test_get_terms_invalid_data_element_URI(
test_app, invalid_data_element_URI
):
"""Given an invalid data element URI, returns a 422 status code as the validation of the data element URI fails."""
response = test_app.get(f"/attributes/{invalid_data_element_URI}")
assert response.status_code == 422
as a reference (although note that the above is a test for invalid parameters, whereas in this case you just need to check that the response from / contains some part of the welcome message you would expect, and maybe a successful status code).

Let me know if you have any questions!

@samadpls samadpls requested a review from alyssadai March 20, 2024 17:47
Copy link
Contributor

@alyssadai alyssadai left a comment

Choose a reason for hiding this comment

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

Thanks @samadpls for the test!

The tests are currently failing on the PR (see https://github.com/neurobagel/api/actions/runs/8363565151/job/22896810427?pr=286#step:6:107), I've suggested an addition below that should fix the problem.

Once the fix is applied, this should be good to merge.

Co-authored-by: Alyssa Dai <[email protected]>
@samadpls samadpls requested a review from alyssadai March 21, 2024 08:54
Copy link
Contributor

@alyssadai alyssadai left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@alyssadai alyssadai merged commit eafd493 into neurobagel:main Mar 21, 2024
5 checks passed
@surchs
Copy link
Contributor

surchs commented Apr 11, 2024

🚀 PR was released in v0.2.0 🚀

@surchs surchs added the released This issue/pull request has been released. label Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
_community [BOT ONLY] PR label for community contributions. Used for tracking pr-minor Non-breaking feature or enhancement, will increment minor version (0.+1.0) released This issue/pull request has been released.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Change root path for API to display an informative landing message/page
4 participants