one of HmsSettings apply functions recurses indefinitely (huawei-push-kit)

[pjfanning]

• Function accidentally calls itself instead of delegating to another apply function.
• the equivalent Java API create method uses the broken apply function
• found by Scala 3 compiler
• fixed by calling default apply function and passing on the default values for the 2 extra params that are needed

[He-Pin]

+100
https://github.com/apache/incubator-pekko-connectors/pull/142/files#diff-833bf8761913c757f04ad1f60fa627e164d98fa6f1bacaad729eb878d2c3931cR74

[He-Pin]

lgtm

[mdedetrich]

ill approve although I don't really like unit tests that are testing just the compiler. Since we are going to integrate Scala 3 into pekko-connectors anyways (which as you said catches this at compile time), I would prefer not having the test or alternately creating an issue to remove it when we add scala 3 support.

[pjfanning]

ill approve although I don't really like unit tests that are testing just the compiler. Since we are going to integrate Scala 3 into pekko-connectors anyways, I would prefer not having the test or alternately creating an issue to remove it when we add scala 3 support.

I'm not testing the compiler - I'm testing that the call completes ok - the test runs into an indefinite loop without my fix

[mdedetrich]

ill approve although I don't really like unit tests that are testing just the compiler. Since we are going to integrate Scala 3 into pekko-connectors anyways, I would prefer not having the test or alternately creating an issue to remove it when we add scala 3 support.

I'm not testing the compiler - I'm testing that the call completes ok - the test runs into an indefinite loop without my fix

Yes I know, I didn't expressive well. What I mean is that I am not a fan of unit tests that are covering for issues that the compiler should detect/show mainly because you can argue "why don't we just add a unit test for every possible apply method that can create an infinite recursive loop". Put differently the test sticks out as a sore thumb, and the impression of someone else reading this test for the first time would be "why is it only this function that has this problem" when this is not the case. This has also revealed that huawei pushkit evidently doesn't have good enough integration test coverage (which would not only uncover this issue but actually test the library in general).

As I said though, I wouldn't be saying this if Scala 3 didn't happen to catch this.

[pjfanning]

@mdedetrich unfortunately, the Scala 3 build just logs a warning - it does not fail

We can play around with the Scala 3 settings to see if we can make the warnings lead to a build failure.

[mdedetrich]

@mdedetrich unfortunately, the Scala 3 build just logs a warning - it does not fail

Unless I am mistaken any warning can be turned into a fatal error, so this shouldn't be an issue

We can play around with the Scala 3 settings to see if we can make the warnings lead to a build failure.

👍