-
Notifications
You must be signed in to change notification settings - Fork 4
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
[IP-387] Upgrade fp-ts
#165
Conversation
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.
I've checked half of the files for now, I just left 3 comments for now
This reverts commit 80cd1c6.
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.
just few other comments
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.
I just found a lot of old _s that could be renamed because they are used and we have to check them.
Some code is not functional, but I understand that it could have been a huge refactoring of old legacy code that's working.
I suggested a refactoring, but my opinion remains that we should just wrap old code since it's working, a refactoring could be a waste of time at this time.
I think that the overall code refactoring should not be so invasive to cause logical problems.
Nice (...and loooong...) job!
const handler = InfoHandler(checkApplicationHealth()); | ||
|
||
return wrapRequestHandler(handler); | ||
return pipe( |
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 is quite amazing. Thanks @gquadrati!
.findOneByServiceId(serviceId) | ||
.run(); | ||
if (isLeft(errorOrMaybeRetrievedService)) { | ||
const errorOrMaybeRetrievedService = await serviceModel.findOneByServiceId( |
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.
Why not improving it with a pipe like the proposed "GetServiceHandler"?
}; | ||
return pipe( | ||
Object.entries(existingGroups), | ||
_ => new Map(_), |
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.
:) I care about _
() => this.container.item(messageId, fiscalCode).delete(), | ||
toCosmosErrorResponse | ||
), | ||
TE.map(_ => _.item.id) |
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.
ditto
() => this.container.item(documentId, notificationId).delete(), | ||
toCosmosErrorResponse | ||
), | ||
TE.map(_ => _.item.id) |
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.
ditto
() => this.container.item(documentId, fiscalCode).delete(), | ||
toCosmosErrorResponse | ||
), | ||
TE.map(_ => _.item.id) |
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.
ditto
) | ||
.map(_ => _.item.id); | ||
), | ||
TE.map(_ => _.item.id) |
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.
ditto
return true; | ||
} | ||
|
||
if ( | ||
deleteResults.some( | ||
_ => _.maybeError.isSome() && _.uResponse.statusCode !== 404 | ||
_ => O.isSome(_.maybeError) && _.uResponse.statusCode !== 404 |
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.
ditto
) | ||
) { | ||
// retry | ||
const errors = new Error( | ||
deleteResults | ||
.map(_ => _.maybeError) | ||
.filter(isSome) | ||
.filter(O.isSome) | ||
.map(_ => _.value.message) |
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.
ditto
Thanks everybody for the huge effort of reviewing this PR, I really appreciate the care you put in refining code so that changes could be as smooth as possible. Basically every review focused on having more clear implementations, and I think code has improved a lot from my first proposal. Furthermore, no behavioral issue emerged, and that makes me way more confident on this refactor. If you agree, I'd stop here with refinements so that we can have a closure on this activity; further improvements can be applied in future as the application evolves. |
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.
LGTM
List of Changes
fp-ts
and all related dependencies in 1e775aeMotivation and Context
We can now move away from legacy, unmaintained
fp-ts@1
How Has This Been Tested?
Unit tests
Screenshots (if appropriate):
Types of changes
Checklist: