Add a new example project with advanced fpdart usecases + update readme #134
Conversation
|
Hi @SandroMaglione. Is there any prerequisite for this PR to get merged? If so, please let me know. |
There was a problem hiding this comment.
@Biplab-Dutta could you expand the README of the example app? Based on it, I will then check the code from a user perspective and see if something is unclear or needs clarification.
After that this can be merged.
There was a problem hiding this comment.
Could you expand the README of the app?
Since this is an example project, the README should guide people on how to read and understand the implementation. This should highlight things like:
- Features of the app
- Package dependencies (and the reason why they are used)
- Overview of the file structure
- Entry point of the app and guidelines on how to read the code
- How
fpdartspecifically is used in the project
There was a problem hiding this comment.
Got it. Shall do it on my weekend.
|
@Biplab-Dutta are you still interested in working on this PR? |
|
Hi @SandroMaglione. |
|
Hi @SandroMaglione. Please check the README for the new example project. |
There was a problem hiding this comment.
Added some comments and suggestions on fpdart code.
After reading the code from a user perspective I am now thinking that this app has too much no-fpdart code. From a user perspective that is looking for fpdart examples having many files is not much help.
Do you think you can reduce the scope and leave mainly fpdart-related files? Otherwise I think it would be better to not include the project directly inside fpdart's examples
| Task<Unit> upsertDetail(DetailDTO detailDTO) { | ||
| final txn = _isar.writeTxn<Unit>( | ||
| () async { | ||
| await _isar.detailDTOs.put(detailDTO); | ||
| return unit; | ||
| }, | ||
| silent: true, | ||
| ); | ||
|
|
||
| return Task(() => txn); |
There was a problem hiding this comment.
This use of Task is incorrect. The function should be declared inside Task, otherwise is not pure:
Task<Unit> upsertDetail(DetailDTO detailDTO) => Task(() { ... })|
|
||
| await _(_headerCache.saveHeader(header).toTaskEither()); | ||
|
|
||
| final html = response.data!; |
There was a problem hiding this comment.
This could potentially fail (using !). I would validate this instead of using ! to make sure the API does not return invalid data.
There was a problem hiding this comment.
In my case, the data could not have been null if the program flow reached there. But change made
| fullPathToMarkdownFile, | ||
| ); | ||
|
|
||
| await _(_headerCache.saveHeader(header).toTaskEither()); |
There was a problem hiding this comment.
Nesting calls using _ may cause problems (read here)
There was a problem hiding this comment.
In my case, how can I have my code refactored? I tried but didn't succeed. I'd appreciate your help on this one.
| return right(ModifiedRemoteResponse(html)); | ||
| } | ||
|
|
||
| return right(const UnModifiedRemoteResponse()); |
There was a problem hiding this comment.
Since you are always returning right this indicates that this TaskEither never fails, better to use Task then
There was a problem hiding this comment.
Something like this?
.flatMap(
(response) {
return Task<RemoteResponse<String>>.Do(
(_) async {
if (response.statusCode == 200) {
final header = GithubHeader.parse(
id,
response,
fullPathToMarkdownFile,
);
await _(_headerCache.saveHeader(header));
final html = response.data!;
return ModifiedRemoteResponse(html);
}
return const UnModifiedRemoteResponse();
},
).toTaskEither();
},
)|
|
||
| return remoteResponse.when( | ||
| noConnection: () async { | ||
| final dto = await _(_localService.getDartDetail(id).toTaskEither()); |
There was a problem hiding this comment.
Same as above, better not use _ in a nested function
| final cachedData = await _( | ||
| _localService.getDartDetail(id).toTaskEither(), |
| _remoteService.getDartChangelogDetail(id), | ||
| ); | ||
|
|
||
| return remoteResponse.when( |
There was a problem hiding this comment.
This returns an async response. You should always use _ when executing async functions inside Do
There was a problem hiding this comment.
I tried but couldn't figure out way to refactor my code. I'd really appreciate your help.
No description provided.