Skip to content
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

Improve customization of service registration #17

Closed
wants to merge 0 commits into from

Conversation

Thomas-X
Copy link
Contributor

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

@Kralizek
Copy link
Owner

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.

@Thomas-X
Copy link
Contributor Author

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

@Kralizek Kralizek self-requested a review February 16, 2021 19:45
@Thomas-X Thomas-X changed the title add the posibility to pass customer serializers (allowing for XML/other format parsing) add the posibility to pass custom serializers (allowing for XML/other format parsing) Feb 17, 2021
@Kralizek
Copy link
Owner

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.

@Thomas-X
Copy link
Contributor Author

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

@Kralizek
Copy link
Owner

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

@Thomas-X
Copy link
Contributor Author

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

@Kralizek
Copy link
Owner

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

@Kralizek Kralizek changed the title add the posibility to pass custom serializers (allowing for XML/other format parsing) Improve customization of service registration Feb 26, 2021
@Kralizek
Copy link
Owner

Kralizek commented Feb 26, 2021

Left to do before merge:

  • Update readme

Left to do before new release:

@Kralizek
Copy link
Owner

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 =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants