search

dfds / dafda

.NET Kafka client library
7 stars 18 forks source link

Made it possible to extend DAFDA with schemas in an external package. #78

Closed faxholm7 closed 1 year ago

faxholm7 commented 1 year ago

Changed internal part to be extendable in other packages. Minor clean up and internal folder restructuring.

jakob-nielsen-dfds commented 1 year ago

With such a big PR it could have been nice with separate commits to logically group changes together. Not the least for formatting changes - removing whitespaces, lines, and adding XML doc comments - it's all noise among the actual changes to the code.

Is it too late to re-write git history to make it more easily reviewable?

jakob-nielsen-dfds commented 1 year ago

After going through the changes it seems to me that this PR is doing the following things:

  1. Adding a whole lot of XML doc comments.
  2. Formatting code - whitespace and newlines.
  3. Changing how ConsumerScope and ConsumerScopeFactory works - a braking change.
  4. Making a base ConsumerConfiguration which can be extended.
  5. Making a lot of internal stuff public - enabling extending DAFDA.

Apart from dividing this into separate commits as mentioned (or even PRs) - I think number 3 should have been a separate PR - it has nothing to do with extending DAFDA.

It might be okay to do a breaking change - but I think we should get other stakeholders' opinions on this. I think it would be nice to have a separate release (a dedicated PR) for changing how ConsumerScope and ConsumerScopeFactory works - so the release with braking change is very focused on the change - not hidden in enabling extensibility of DAFDA.