dasch / avro_turf

A library that makes it easier to use the Avro serialization format from Ruby.
MIT License
167 stars 80 forks source link

Add RBS types, some refactoring and some small BC breaks #168

Closed piotaixr closed 2 years ago

piotaixr commented 2 years ago

I'm trying to generalize the use of RBS typings and the [steep]() typechecker at my company and we are using avro_turf.

In order to avoid the necessity to vendor the types of the libraries we use in our projects, and as steep is able to get the types from the used gems, I tried to add them to avro_turf.

There were some problems that I had to fix, though:

Also, not related to typings, but more on having the smallest impact on the environment when using the gem, I rewrote the CoreExt monkeypatchs using a Refinement. Why?

  1. Monkeypatching is bad
  2. The fixes for the tests have a breaking change (requires ruby >= 2.5 + others) so why not group them in a single release?

@dasch Are you willing to review/merge these kind of changes? All changes are in this branch of my fork so you can take a look at the whole patch (and I can create multiple PRs with them if necessary). For now, the strict diagnostics are enabled, but curiously, the default (not specifically enabling any diagnostic) is stricter. I'll see if I can narrow the types down more.

Thanks!

dasch commented 2 years ago

I'm not keen on the refinement stuff; the DecodedMessage change should be backwards compatible in practice, since I doubt people are actually using the enum aspects. Can you break the changes into small PRs perhaps?

piotaixr commented 2 years ago

@dasch I did a first round of PRs with the first changes. You can see the 'finished' work (almost) in my feature/rbs-and-steep branch.

I'm not keen on the refinement stuff

Can you elaborate more on that? I've been doing Ruby for 4 years and have never seen refinements really used anywhere. And I don't understand why the community seems to prefer patching the environment globally (or even depend on activesupport/core_ext -- ewww) as doing this is a no-go for all other programming languages I've used. To go back on the particular use case we have here, it feels right doing it this way.

dasch commented 2 years ago

Well, as a library author I only care about what people actually use 😆

If Refinements aren't in widespread use, I see no reason to make backwards incompatible changes to adopt them. The current way works just fine for the use cases I've seen.

github-actions[bot] commented 2 years ago

Stale issue message