Correctly support Ruby 2.7+ keyword arguments.

[ioquatix]

honeybadger-ruby/lib/honeybadger/agent.rb

Line 121 in 1052fd6

def notify(exception_or_opts, opts = {})

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 refactor!: 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.

[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 e4a006c 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.