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

Kpi values are not string but specific classes #25

Merged
merged 3 commits into from
Sep 21, 2023
Merged

Kpi values are not string but specific classes #25

merged 3 commits into from
Sep 21, 2023

Conversation

benoit74
Copy link
Collaborator

@benoit74 benoit74 commented Sep 19, 2023 •
edited

Rationale

Fix #16

Changes

  • Kpi values are now typed properly, for easier usage throughout the codebase
  • serialization/deserialization is transparently handled at the DB level (via https://docs.sqlalchemy.org/en/20/core/custom_types.html)
  • serialization/deserialization is transparently handled at the API level
  • update pyright from 1.1.325 to 1.1.327

@benoit74 benoit74 self-assigned this Sep 19, 2023
@benoit74 benoit74 changed the title Replace strings Kpi values are not string but specific classes Sep 19, 2023
@codecov
Copy link

codecov bot commented Sep 19, 2023 •
edited

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.10% 🎉

Comparison is base (e857f40) 97.77% compared to head (fee52f3) 97.88%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   97.77%   97.88%   +0.10%     
==========================================
  Files          24       24              
  Lines         763      802      +39     
  Branches      168      170       +2     
==========================================
+ Hits          746      785      +39     
  Misses         12       12              
  Partials        5        5
Files Changed Coverage Δ
...etrics_backend/business/kpis/content_popularity.py 100.00% <100.00%> (ø)
...d/src/offspot_metrics_backend/business/kpis/kpi.py 100.00% <100.00%> (ø)
...offspot_metrics_backend/business/kpis/processor.py 100.00% <100.00%> (ø)
...src/offspot_metrics_backend/business/kpis/value.py 100.00% <100.00%> (ø)
backend/src/offspot_metrics_backend/db/models.py 100.00% <100.00%> (ø)
...ackend/src/offspot_metrics_backend/db/persister.py 100.00% <100.00%> (ø)
...src/offspot_metrics_backend/routes/aggregations.py 100.00% <100.00%> (ø)
backend/src/offspot_metrics_backend/routes/kpis.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@benoit74 benoit74 force-pushed the replace_strings branch 3 times, most recently from f5a48fd to f5498ee Compare September 19, 2023 13:35
rgaudin
rgaudin approved these changes Sep 20, 2023
View reviewed changes
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

Ah it's a lot more satisfying now. A few inline comments. I think alembic should be next to the migrations folder.

backend/src/offspot_metrics_backend/db/models.py
class KpiValueType(types.TypeDecorator[KpiValue]):
"""Handle KpiValue serialization/deserialization in a transparent way."""

impl = types.String
Copy link
Member

Choose a reason for hiding this comment

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

Is this a pydanctic metadata?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is SQLAlchemy metadata (linked to the types.TypeDecorator class) indicating which type will be sent to / received from the DB. I should probably have mentioned https://docs.sqlalchemy.org/en/20/core/custom_types.html in the PR, I just added it.

backend/src/offspot_metrics_backend/db/models.py Show resolved Hide resolved
backend/src/offspot_metrics_backend/db/models.py Show resolved Hide resolved
backend/tasks.py
@@ -55,7 +55,7 @@ def call_alembic(ctx: Context, cmd: str, *, test_db: bool = False):
# this won't be needed once https://github.com/pyinvoke/invoke/issues/170 will be
# implemented and we will be able to call alembic task from other related tasks

with ctx.cd("src"):
with ctx.cd("src/offspot_metrics_backend"):
Copy link
Member

Choose a reason for hiding this comment

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

ugh. I think alembic.ini is misplaced and should be in src/. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change is very intentional.

It is not the "usual" location but is needed for us so that this ini file is embedded in the Python package at build, installed with pip install, and available for migration execution / checks when the process starts.

I find it cleaner that customizing the build to include this file, since it means we will also need to specify target location and stuff like that which make it complex for little added value from my PoV. But I'm very open to suggestion on this, I do not have much experience.

Copy link
Member

Choose a reason for hiding this comment

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

Well that's rude. I opened this comment and your gave your opinion without changing the code. It is understood that I'd be resolving the conversation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm very sorry about that. I really don't get the approval / resolve conversation PR process. It is very blurry / not understandable for me when to resolve or not the conversations, and also frustrating to not know how to do a good job. Maybe it is understood for you that you'll be resolving the conversation, not for me, sorry.

I've opened an issue to discuss openly about this: openzim/overview#22

Please, feel free to reply to any pending conversations on this PR, I've re-opened unresolved conversation and will take them into account

@benoit74 benoit74 merged commit ae7e8d1 into main Sep 21, 2023
6 checks passed
@benoit74 benoit74 mentioned this pull request Sep 21, 2023
Closed
@benoit74 benoit74 deleted the replace_strings branch September 21, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgaudin rgaudin rgaudin approved these changes

Assignees

@benoit74 benoit74

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Do not use str to store KPI values
2 participants
@benoit74 @rgaudin