Add :name config option to Supervisor

[jeroenvisser101]

When starting Money.ExchangeRate.Supervisor with the :restart option, the supervisor is expected to be named. The option was dropped in v5.0.2 but is still required.

I'm not sure if this was deleted on purpose, or if it seemed obsolete.

Thanks! We love ex_money :)

[kipcole9]

Thanks for the PR. I manually merged it and published ex_money. Changelog:

Bug Fixes

• Add back the name of the Application supervisor, Money.Supervisor. Thanks for the report of the regression to @jeroenvisser101. Fixes Add :name config option to Supervisor #117.

[jeroenvisser101]

@kipcole9 thank you!

I wasn't able to come up with a good plan for testing this, and couldn't find any similar tests in the test suite. Do you think it's useful to add this?

[kipcole9]

Definitely a regression, apologies for the inconvenience. It came about during some refactoring to use the "new style" GenServer child specs.

Good point about the test - I don't have any smart ideas but I'll think on it over another coffee :-)

[jeroenvisser101]

Definitely a regression, apologies for the inconvenience. It came about during some refactoring to use the "new style" GenServer child specs.

No problem at all! And happy to be able to contribute back :)

I may have another PR coming up in CLDR re: :round_nearest KeyError, but I'm gonna dive into that now

Thanks again! 🙏

[kipcole9]

On reflection the way to test is to abstract the configuration of the supervisor which I have now done. I have re-published version 5.2.1 so you will have to reinstall it. No functional difference but I'm happier that the supervisor configuration is now in mix.exs. This would allow a library consumer to start ex_money manually with their own configuration for the supervisor. This also allowed me to add a test. Updated changelog:

Enhancements

• Configure the Money.Application supervisor via the arguments to Money.Application.start/2 and configure defaults in mix.exs. This permits different restart strategies and names.

Bug Fixes

• Add back the name of the Application supervisor, Money.Supervisor. Thanks for the report of the regression to @jeroenvisser101. Fixes Add :name config option to Supervisor #117.

[kipcole9]

I may have another PR coming up in CLDR re: :round_nearest KeyError, but I'm gonna dive into that now

Please do log an issue over at ex_cldr_numbers or here if you have a reproducible case.

[jeroenvisser101]

@kipcole9 should we also add this to this line so it would match the configuration?

money/lib/money/exchange_rates/exchange_rates_supervisor.ex

Line 75 in 4b9c097

Supervisor.terminate_child(Money.Supervisor, __MODULE__)

[jeroenvisser101]

Please do log an issue over at ex_cldr_numbers or here if you have a reproducible case.

Will do! I think it's related to elixir-cldr/cldr_numbers@3bc8291, expect an issue/PR soon :)

[kipcole9]

Good call on the supervisor termination. I re-published again 5.2.1 with updated changelog. Very hacky release process this morning (my time). Updated changelog:

Enhancements

• Configure the Money.Application supervisor via the arguments to Money.Application.start/2 and configure defaults in mix.exs. This permits different restart strategies and names.

• Add Money.ExchangeRates.Supervisor.default_supervisor/0 to return the name of the default supervisor which is Money.Supervisor

• Change Money.ExchangeRates.Supervisor.stop/0 to become Money.ExchangeRates.Supervisor.stop/{0, 1} allowing the supervisor name to be passed in. The default is Money.ExchangeRates.Supervisor.default_supervisor/0

Bug Fixes

• Add back the name of the Application supervisor, Money.Supervisor. Thanks for the report of the regression to @jeroenvisser101. Fixes Add :name config option to Supervisor #117.

[jeroenvisser101]

Seems that I was wrong and it just required deps.clean, all is good, although I did notice this:
https://github.com/elixir-cldr/cldr_numbers/blob/3bc8291ccc6bf8849bebe89c7bdf2b1296b922db/lib/cldr/number/format/compiler.ex#L262

I believe that should also be updated to round_nearest, correct?

[jeroenvisser101]

Very hacky release process this morning (my time). Updated changelog:

That's how we like it best haha, good morning :)

[kipcole9]

I believe that should also be updated to round_nearest, correct?

Most likely. I need to create a test case.

[jeroenvisser101]

Most likely. I need to create a test case.

I'm running the tests with the change as we speak, but 566 locales seems to be quite a lot, even on 16 cores

[kipcole9]

it takes maybe 30 minutes on my iMac Pro of similar configuration. Thats why there is a warning against configuring all locales :-)

[kipcole9]

Moving this conversation to https://github.com/elixir-cldr/cldr_numbers