AWSLambdaSharpTemplate icon indicating copy to clipboard operation
AWSLambdaSharpTemplate copied to clipboard

Improve customization of service registration

Open Thomas-X opened this issue 3 years ago • 8 comments

The idea for this PR is so you can pass your own serialization logic incase your incoming messages are in an other format than JSON.

I added unit tests too, but let me know if there's any questions or remarks on how it's structured. README is updated to reflect the (optional) API changes as well

Thomas-X avatar Feb 15 '21 16:02 Thomas-X

Hi @Thomas-X Thank you for your pull request. I will be checking the PR thoroughly sometimes this week (hopefully already tomorrow but I can't promise).

In the meantime, I would appreciate if you could revert some codestyle changes that slipped through when you committed your changes.

I'm asking this because it makes it easier to understand what was changed in the PR.

Kralizek avatar Feb 15 '21 18:02 Kralizek

Changed some of the formatting that slipped through, maybe it could be worth adding a .editorconfig for the future

Thomas-X avatar Feb 16 '21 08:02 Thomas-X

Hi @Thomas-X, sorry for the delay.

I have been thinking a bit about this PR and I think that adding (yet) another parameter to the registration methods is not sustainable in the long term.

I'm considering the idea of using a configuration delegate that can be used to customize the registered services as needed.

Something like below

services.UseNotificationHandler(c => c.UseSerializer<MyCustomSerializer>().EnableParallelExecution());

So my plan is to rewrite this PR with this design but I am not sure when I have time to do so.

Kralizek avatar Feb 26 '21 09:02 Kralizek

Hi @Kralizek, no problem, we all have lives :).

I think that using a configuration delegate definitely makes it more future-proof and more extensible, so we should rewrite this PR for that.

I'm a bit dry on time as well, but I can take a look at the idea next week(end) probably, and see if I'm able to implement it! (I'm a junior dev). Unless you beat me to the punch, of course

Thomas-X avatar Feb 26 '21 09:02 Thomas-X

@Thomas-X managed to squeeze a nick of time for the lib part. tests need to be updated as the build is failing now.

Kralizek avatar Feb 26 '21 12:02 Kralizek

@Kralizek That's really awesome, I'll see what I can do soon to get it built and the tests green

Thomas-X avatar Feb 26 '21 12:02 Thomas-X

Could you add an implementation of ISerializer for JSON so that we skip that ugly if?

Kralizek avatar Feb 26 '21 13:02 Kralizek

Left to do before merge:

  • [ ] Update readme

Left to do before new release:

  • [ ] Update CI/CD #18
  • [ ] (optional) Convert unit tests to use AutoFixture and Moq #19

Kralizek avatar Feb 26 '21 16:02 Kralizek

Hey @Thomas-X I was working on this lib and by mistake I fucked up your PR while rebasing it after I merged in #22

I am not able to reopen this PR. I have the code locally but I'd love if you would open a new PR so that you can take credit on the work you did.

Best regards and sorry =)

Kralizek avatar Oct 27 '22 17:10 Kralizek