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

editoast: create editoast_models and move model v2 database tools #7864

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

Wadjetz
Copy link
Member

@Wadjetz Wadjetz commented Jun 27, 2024

No description provided.

@Wadjetz Wadjetz self-assigned this Jun 27, 2024
@Wadjetz Wadjetz added the area:editoast Work on Editoast Service label Jun 27, 2024
@Wadjetz Wadjetz force-pushed the ebe/editoast-models-lib branch from abb5a0a to b1e70bc Compare June 27, 2024 12:44
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 87.50000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 28.41%. Comparing base (4e8be63) to head (e14e6f5).
Report is 7 commits behind head on dev.

Files Patch % Lines
editoast/editoast_models/src/db_connection_pool.rs 62.50% 3 Missing ⚠️
editoast/src/generated_data/error/mod.rs 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #7864      +/-   ##
============================================
- Coverage     28.45%   28.41%   -0.04%     
  Complexity     2059     2059              
============================================
  Files          1256     1264       +8     
  Lines        154603   155097     +494     
  Branches       3049     3053       +4     
============================================
+ Hits          43989    44078      +89     
- Misses       108797   109198     +401     
- Partials       1817     1821       +4     
Flag Coverage Δ
core 75.00% <ø> (ø)
editoast 71.62% <87.50%> (+0.02%) ⬆️
front 9.99% <ø> (+0.02%) ⬆️
gateway 2.34% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 72.93% <ø> (ø)

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

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

@Wadjetz Wadjetz force-pushed the ebe/editoast-models-lib branch 4 times, most recently from f304a47 to e7046be Compare July 1, 2024 13:22
@Wadjetz Wadjetz requested review from woshilapin and leovalais July 1, 2024 13:44
@Wadjetz Wadjetz marked this pull request as ready for review July 1, 2024 13:44
@Wadjetz Wadjetz requested a review from a team as a code owner July 1, 2024 13:44
@Wadjetz Wadjetz changed the title editoast: move model v2 database tools to editoast_models editoast: create editoast_models and move model v2 database tools Jul 1, 2024
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

LGTM

editoast/editoast_models/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

Yes, I did it, I reviewed it all 🎉 Thank you for this refactorization. Don't forget the license.

editoast/editoast_models/src/database/connection_pool.rs Outdated Show resolved Hide resolved
editoast/editoast_models/src/lib.rs Outdated Show resolved Hide resolved
editoast/editoast_models/Cargo.toml Show resolved Hide resolved
editoast/src/views/test_app.rs Outdated Show resolved Hide resolved
@Wadjetz Wadjetz force-pushed the ebe/editoast-models-lib branch 4 times, most recently from c70033b to 9b12628 Compare July 2, 2024 12:32
Copy link
Contributor

@leovalais leovalais left a comment

Choose a reason for hiding this comment

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

Thanks for this big refactoring 😄
(I don't believe I've checked this many "viewed" since the switch to diesel_async 😅)

I'll summarize here what we discussed yesterday so that we're all on the same page.

On errors

  1. Although the crate editoast_models now exists, we still can't move models into it as they all rely on InternalError.
  2. The first step would be to redesign the failures of ModelV2's API.
  3. Then we'd have to figure out a way to forward the information of the errors defined in sub-crates up to editoast_views without manually reading all fields of all variants.
  4. Only then will we be able to start moving models around.

On diesel* dependencies

The goal is to remove the diesel dependencies in all crates but editoast_models.

  1. We need to finalize the pool V2 and tests migration (which depends on dropping TSV1 ofc 🙃)
  2. The DbConnection type must be made opaque.
  3. All db queries in the views must be moved into the most relevant model.

That's all I believe?

editoast/editoast_models/src/database/connection_pool.rs Outdated Show resolved Hide resolved
editoast/editoast_models/src/lib.rs Outdated Show resolved Hide resolved
@Wadjetz Wadjetz force-pushed the ebe/editoast-models-lib branch 5 times, most recently from 19b6b2f to ff82bba Compare July 3, 2024 08:27
@Wadjetz Wadjetz force-pushed the ebe/editoast-models-lib branch from ff82bba to 4c8a295 Compare July 3, 2024 08:42
@Wadjetz Wadjetz requested a review from woshilapin July 3, 2024 13:11
@woshilapin woshilapin enabled auto-merge July 3, 2024 13:27
@Wadjetz Wadjetz force-pushed the ebe/editoast-models-lib branch from ca951fa to 54d6a65 Compare July 3, 2024 14:59
@woshilapin woshilapin force-pushed the ebe/editoast-models-lib branch from 54d6a65 to e14e6f5 Compare July 3, 2024 15:00
@woshilapin woshilapin added this pull request to the merge queue Jul 3, 2024
Merged via the queue into dev with commit 4acb3a6 Jul 3, 2024
17 checks passed
@woshilapin woshilapin deleted the ebe/editoast-models-lib branch July 3, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:editoast Work on Editoast Service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants