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: add patch for sbl institution types #110

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

lchen-2101
Copy link
Collaborator

closes #107

Copy link

github-actions bot commented Mar 6, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/entities/models
  dao.py
  dto.py
  src/entities/repos
  institutions_repo.py
  repo_utils.py
  src/routers
  institutions.py
Project Total  

This report was generated by python-coverage-comment-action

case "hmda":
raise HTTPException(status_code=HTTPStatus.NOT_IMPLEMENTED, detail="HMDA type not yet supported")


Copy link
Contributor

Choose a reason for hiding this comment

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

Just the question I put in Mattermost when you lost power:

Wondering if we should be returning 204 NO_CONTENT instead of None? I've done that with a few of the endpoints in filing, and added it to the wiki for others to bring up if we want to decide on a consistent approach

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeap, the sbl case I've updated to use no_content instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmda one is a question mark at the moment; if we do allow more open modifications later down the line, I'm guessing the hmda one will be more like the other normal fields, rather than the special case types dealing

Copy link
Contributor

@jcadam14 jcadam14 left a comment

Choose a reason for hiding this comment

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

LGTM

@jcadam14 jcadam14 merged commit 76202f5 into main Mar 6, 2024
3 checks passed
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.

Create patch endpoint for updating institution types
2 participants