sentry-cli icon indicating copy to clipboard operation
sentry-cli copied to clipboard

Refactor `src/api.rs`

Open szokeasaurusrex opened this issue 1 year ago • 0 comments

We should split the code into multiple sub-modules to improve code organization, and simplify the code along the way if possible. We will split the work into two phases.

Phase 1 – current phase

In this phase, we focus on splitting off code that we can relatively easily move out of the main src/api/mod.rs file into separate submodules, with little to no modification. We can consider simplifying any overly complex split out code if it is easy to do so, but the main focus should just be to reduce the line count in the main src/api/mod.rs file by moving things that don't need to be there out of the file.

### Subtasks
- [x] Split helper types (e.g. error types and pagination logic) into separate submodules
- [ ] https://github.com/getsentry/sentry-cli/issues/2077
- [ ] https://github.com/getsentry/sentry-cli/issues/2079
- [ ] https://github.com/getsentry/sentry-cli/issues/2078

Why should we implement the proposed changes in this phase?

The main benefit will be to make development on the src/api portion of the Sentry CLI codebase easier. src/api/mod.rs is over 2500 lines long, and the file is doing many different things. Thus, it is difficult for developers to find where they need to make changes when fixing a bug/implementing a new feature.

In the sub-issues above, I have identified several areas of the file's code that have very clear distinct responsibilities, and can therefore relatively easily be split into separate files. Separating items into separate files makes it easier for devs to find the code they need to change without being overwhelmed, and hopefully sets a precedent that future new functionality is split into its own file rather than making a very long file even longer. And it is easier to write tests when things are separated out.

What are the risks of implementing the proposed changes?

The risk is that we could break something if we make a mistake when moving code around.

We can mitigate this risk by manually testing our changes before publishing them. We can also mitigate the risk by limiting changes during this refactor to simply moving code around – if we also attempt to simplify code, we greatly increase the risk of making a mistake.

Phase 2 – under consideration

After completing phase 1, we will have greatly improved the api module by moving logic that can live in a submodule out of the main module. This should reduce the main module's original size by about half – this is a great improvement, but with ~1500 lines, the file is still a bit long.

Phase 2 is, however, less simple that Phase 1, because we can no longer gain much by splitting the remaining Api, AuthenticatedApi, and RegionSpecificApi structs into separate files – AuthenticatedApi itself is ~1000 lines. We likely need to change the architecture of how this part of our codebase works, so we can move things into separate submodules.

The value of completing Phase 2 is perhaps more questionable than Phase 1 – it likely requires more work since we need to rethink how the code should work, and there is the risk of breaking things in the process. The effort could pay off though if the code becomes easier to understand and add to.

We will plan this phase in more detail after completing Phase 1, but this phase could include the following:

  • [ ] Refactor logic in Api, AuthenticatedApi, and RegionSpecificApi, so the logic can be split into separate submodules
  • [ ] All low-level functions (i.e. functions that directly call the curl library to make the request) should be kept in one (ideally small) sub-module, so that we can easily migrate to reqwest by only changing one submodule (see #1897). We could achieve this by creating a lightweight wrapper around curl, and having the rest of the code only call the wrapper instead of curl directly.
  • [ ] Consider getting rid of static reference. This may be necessary if we decide to eliminate the Api struct completely during the refactor, but even if we keep it, it could be simpler just to have every command generate it on demand.

szokeasaurusrex avatar Apr 02 '24 14:04 szokeasaurusrex