Correctly support Ruby 2.7+ keyword arguments.

[ioquatix]

https://github.com/honeybadger-io/honeybadger-ruby/blob/1052fd6ab7d9d5a138bd9d1f64c4a077bf980193/lib/honeybadger/agent.rb#L121

Should probably be def notify(exception_or_opts, **opts).

[joshuap]

Thanks @ioquatix! I think we could support this once we drop support for Ruby 2.6 (which officially ended yesterday, I see. 😁)

Related: #427

[halfbyte]

@joshuap I've started to take a look at this and obviously this is not a super trivial change. The signature that would most likely work is notify(exception = nil, **opts) and I can get a ton of tests to work with that.

What I am more worried about is people using explicit hashes ala notify({error_message: "yeah"}) and I wonder if we should make this a breaking change or if the code needs to somehow take care of these "legacy calls".

What's your thoughts on this?

EDIT: I think I can make it work with a signature like this (which unfortunately makes it a bit more complicated than before): notice(exception_or_opts=nil, opts = {}, **kwopts). This should catch all cases where explicit hashes are given it and at the same time work with kwargs just fine.

[joshuap]

@joshuap I've started to take a look at this and obviously this is not a super trivial change. The signature that would most likely work is notify(exception = nil, **opts) and I can get a ton of tests to work with that.

What I am more worried about is people using explicit hashes ala notify({error_message: "yeah"}) and I wonder if we should make this a breaking change or if the code needs to somehow take care of these "legacy calls".

What's your thoughts on this?

EDIT: I think I can make it work with a signature like this (which unfortunately makes it a bit more complicated than before): notice(exception_or_opts=nil, opts = {}, **kwopts). This should catch all cases where explicit hashes are given it and at the same time work with kwargs just fine.

I think this is OK for now, although I may think about it a bit before releasing. My gut says your approach is best (albeit more complicated) until the next major/breaking release, when we should plan to remove the second opts argument and move to keyword arguments exclusively. What do you think @subzero10?

[subzero10]

@joshuap To clarify:

• you are suggesting to go ahead with @halfbyte's suggestion for a temporary signature -> notice(exception_or_opts=nil, opts = {}, **kwopts)
• and some time after that, proceed to release the final version of the signature with #499

I think that's a good approach, though I admit I can't size "complexity" here since my ruby skills are limited.

And if I'm not adding a lot to the complexity of the temporary signature, I would suggest logging a warning when the method is called with "legacy" params, something like: Passing params as an object is deprecated and will not work in future releases. Please click here to read more.

[joshuap]

@joshuap To clarify:

• you are suggesting to go ahead with @halfbyte's suggestion for a temporary signature -> notice(exception_or_opts=nil, opts = {}, **kwopts)
• and some time after that, proceed to release the final version of the signature with chore!: Remove second opts argument in Honeybadger.notify #499

I think that's a good approach, though I admit I can't size "complexity" here since my ruby skills are limited.

And if I'm not adding a lot to the complexity of the temporary signature, I would suggest logging a warning when the method is called with "legacy" params, something like: Passing params as an object is deprecated and will not work in future releases. Please click here to read more.

The other option is to merge #499 and do a major release (6.0). Or, if we're not quite ready for 6.0, we could also revert https://github.com/honeybadger-io/honeybadger-ruby/commit/e4a006cfb2a2ecbab2f742b6e9f9c8e9b8958430 and plan to fix it for real in the next major release (without the interim step). I think I prefer the latter.

[joshuap]

I'm also fine with a deprecation warning if we decide to keep the temporary signature and do a minor release.

[ioquatix]

This looks great, nice work team.

[subzero10]

I have no strong opinion about either approach. We haven't released the ruby gem since March 2023, which could be +1 for a major release 🤷 . Or you could argue the same for the opposite: let's make a release for the fixes that haven't been released since some time and plan the major release.

Actually, the more I think about it, I'm leaning towards the major release: no real winner, so I'd go for the simpler solution. Would you be OK with that @joshuap and @JanStevens ?

[joshuap]

I have no strong opinion about either approach. We haven't released the ruby gem since March 2023, which could be +1 for a major release 🤷 . Or you could argue the same for the opposite: let's make a release for the fixes that haven't been released since some time and plan the major release.

Actually, the more I think about it, I'm leaning towards the major release: no real winner, so I'd go for the simpler solution. Would you be OK with that @joshuap and @JanStevens ?

One thing to consider is that we'll need to do a release once we finish the logging/insights work—will that also need to be a major release, or can it be a minor one? I'd prefer not to release two major versions back-to-back.

[subzero10]

a release once we finish the logging/insights work

If it won't introduce breaking changes, it can be a minor release. If it has breaking changes, then I guess we can skip the major release for this one.