-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
Refactor src/api.rs
#2008
Labels
Comments
szokeasaurusrex
added a commit
that referenced
this issue
Apr 2, 2024
This change will allow us to split logic off from src/api/mod.rs into separate files. Eventually, the src/api/mod.rs file should contain only mod statements and re-exports from submodules, which we will create in future PRs. ref #2008
This was referenced Apr 2, 2024
szokeasaurusrex
added a commit
that referenced
this issue
Apr 3, 2024
szokeasaurusrex
added a commit
that referenced
this issue
Apr 4, 2024
szokeasaurusrex
added a commit
that referenced
this issue
Apr 9, 2024
By using `Infallible`, the type signature guarantees an error can never occur ref #2008
szokeasaurusrex
added a commit
that referenced
this issue
Apr 9, 2024
szokeasaurusrex
added a commit
that referenced
this issue
Apr 9, 2024
By using Infallible, the type signature guarantees an error can never occur ref #2008
This was referenced Apr 9, 2024
szokeasaurusrex
added a commit
that referenced
this issue
Apr 9, 2024
This commit adds a new submodule to the API module, errors, which contains error types. Future PRs will split the errors module into submodules for each error type. ref #2008
szokeasaurusrex
added a commit
that referenced
this issue
Apr 9, 2024
szokeasaurusrex
added a commit
that referenced
this issue
Apr 9, 2024
szokeasaurusrex
added a commit
that referenced
this issue
Apr 23, 2024
This code is more closely related to the utils module's progress bar code than to the api code. Ref #2008
szokeasaurusrex
added a commit
that referenced
this issue
Apr 24, 2024
This code is more closely related to the utils module's progress bar code than to the api code. Ref #2008
szokeasaurusrex
added a commit
that referenced
this issue
May 28, 2024
This code is more closely related to the utils module's progress bar code than to the api code. Ref #2008
szokeasaurusrex
added a commit
that referenced
this issue
May 28, 2024
This code is more closely related to the utils module's progress bar code than to the api code. Ref #2008
szokeasaurusrex
added a commit
that referenced
this issue
May 28, 2024
This code is more closely related to the utils module's progress bar code than to the api code, so we should move it there. Ref #2008
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
ApiRequest
andApiResponse
into separate submodules #2079Why 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
, andRegionSpecificApi
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:
Api
,AuthenticatedApi
, andRegionSpecificApi
, so the logic can be split into separate submodulesreqwest
by only changing one submodule (see Replacecurl
withreqwest
#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.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.The text was updated successfully, but these errors were encountered: