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

feat: Adds localization column in the alerts table #289

Closed
wants to merge 5 commits into from
Closed

feat: Adds localization column in the alerts table #289

wants to merge 5 commits into from
+204 −21

Conversation

blenzi
Copy link
Contributor

@blenzi blenzi commented Oct 3, 2023

Following (#267), this PR adds a localization field in the alerts table, including minimal necessary changes.
The localization field is meant to be used as a JSON dump of a list of bounding boxes.

@github-actions github-actions bot added topic: docs Improvements or additions to documentation ext: client module: schemas module: models labels Oct 3, 2023
@codecov
Copy link

codecov bot commented Oct 3, 2023 •
edited
Loading

Codecov Report

Merging #289 (ba2dcbd) into main (ca604f7) will decrease coverage by 0.16%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##             main     #289      +/-   ##
==========================================
- Coverage   95.35%   95.20%   -0.16%     
==========================================
  Files          66       66              
  Lines        1571     1605      +34     
==========================================
+ Hits         1498     1528      +30     
- Misses         73       77       +4
Flag Coverage Δ
client 98.85% <93.33%> (-1.15%) ⬇️
unittests 94.99% <86.95%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/app/models/alert.py 95.45% <100.00%> (+0.21%) ⬆️
client/pyroclient/client.py 98.70% <93.33%> (-1.30%) ⬇️
src/app/schemas/alerts.py 90.90% <85.71%> (-9.10%) ⬇️

@blenzi blenzi requested review from MateoLostanlen and a team October 3, 2023 12:54
MateoLostanlen
MateoLostanlen reviewed Oct 3, 2023
View reviewed changes
Copy link
Member

@MateoLostanlen MateoLostanlen left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR, it might be nice to have a test with a different localization than None.

MateoLostanlen
MateoLostanlen reviewed Oct 3, 2023
View reviewed changes
Makefile Outdated Show resolved Hide resolved
feat: add validation for localization in alert schema and tests
cf00938 
- fix localization field in test_alerts.py
- client: convert_loc_to_str raise ValueError instead of empty list by default
@blenzi
Copy link
Contributor Author

blenzi commented Oct 3, 2023

Thanks @MateoLostanlen . There is a value that is not None but it was not according to the desired format (conf missing). I added some validation checks and fixed it

@blenzi blenzi mentioned this pull request Oct 18, 2023
Merged
@frgfm frgfm added the duplicate This issue or pull request already exists label Oct 26, 2023
@frgfm
Copy link
Member

frgfm commented Nov 3, 2023

Closing this since it has been merged in #267

@frgfm frgfm closed this Nov 3, 2023
@frgfm frgfm deleted the alert_bboxes branch November 3, 2023 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MateoLostanlen MateoLostanlen MateoLostanlen left review comments

Assignees
No one assigned
Labels
duplicate This issue or pull request already exists ext: client/tests ext: client ext: tests module: models module: schemas topic: docs Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blenzi @frgfm @MateoLostanlen