search

rust-lang / bors

Rust implementation of bors used for various Rust components (e.g. the compiler).
Apache License 2.0
46 stars 20 forks source link

refactor: clean up RepositoryLoader #180

Closed vohoanglong0107 closed 1 day ago

vohoanglong0107 commented 1 week ago

Why

Previously RepositoryLoader was created to ease the migration from trait to sqlx. Now that the migration is complete, we can safely clean it up.

vohoanglong0107 commented 4 days ago

Oh that makes perfect sense. You want me to document this into the code, to prevent future "clean up" attempts? 😆

On Mon, Nov 25, 2024, 17:39 Jakub Beránek @.***> wrote:

@.**** commented on this pull request.

Left one comment, otherwise the cleanup looks nice.

In src/github/api/mod.rs https://github.com/rust-lang/bors/pull/180#discussion_r1856116018:

             app.clone(),

installation_client.clone(), team_api_client, repo.clone(), name.clone(), ) .await

  • .map_err(|error| {
  • anyhow::anyhow!("Cannot load repository {:?}: {error:?}", repo.full_name)
  • });
  • {
  • Ok(repo_state) => repo_state,

The complicated handling of results here was by design, because we wanted to have a hard error when the repos fail to load when the bot starts, but only log a warning when we reload the state during the bot's execution. By moving the error handling here, it is not possible to distinguish these two cases. So I would prefer keeping the previous approach of returning Result.

— Reply to this email directly, view it on GitHub https://github.com/rust-lang/bors/pull/180#pullrequestreview-2457649961, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASTX42DI6CGFGWI2GNFPFX32CLO4LAVCNFSM6AAAAABSJFFWNOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINJXGY2DSOJWGE . You are receiving this because you authored the thread.Message ID: @.***>

Kobzol commented 4 days ago

Yes please :)