-
Notifications
You must be signed in to change notification settings - Fork 83
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
chore(request-client): expose NoPersistHttpDataAccess #1540
Conversation
WalkthroughThe pull request introduces a new import and export of Changes
Note: No sequence diagram is generated as the changes are minimal and do not involve complex interactions or new functionality. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
will re-review again after having more context
I'll take a look at this. Please don't merge until I've wrapped my head around it. |
Well noted @MantisClone . But it looks like I'm going to submit another PR. Apparently, using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change could be useful to other app builders who use RequestNetworkBase
directly, and I'm in favor of merging it. Moreover, I think it matches the existing convention - The other DataAccess
implementations are exported so NoPersistHttpDataAccess
should be exported too.
@kevindavee can you share why |
At Request Finance, we're using our own custom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed internally with @kevindavee and this one might not fit our needs because it only applies to the HttpDataAccess
, whereas we need it a lever higher, at the DataAccess
level. Dave has started working on a more generic solution in #1541
Sounds good. Can we still merge this PR? |
Replaced by #1541 |
Description of the changes
There is no way to use
NoPersistHttpDataAccess
without using theRequestNetwork
class. If we want to injectNoPersistHttpDataAccess
, we can't use it withRequestNetworkBase
.Example: I want to use my own custom data access or
NoPersistHttpDataAccess
depending on my use case.RequestNetwork
, there's no way to inject my own custom data access.RequestNetworkBase
, there's no option to useskipPersistence
which usesNoPersistHttpDataAccess
.By exporting
NoPersistHttpDataAccess
, I can manage which Data Access I want to use (Custom/NoPersist) and inject it by myself toRequestNetworkBase
.Summary by CodeRabbit