Fix configure example in readme

kipcole9 commented 1 year ago

@krns thank you very much, greatly appreciated. I know I am badly behind in finishing up the work on localized verified paths but I'll be finishing that up next week for sure.

kipcole9 commented 1 year ago

Published ex_cldr_routes version 0.6.3 with the following changelog entry:

Bug Fixes

Fix readme example. Thanks to @krns for the PR. Closes #12.
Support Phoenix 1.7. NOTE: Doesn't yet include localized verified routes.

krns commented 1 year ago

Because you mentioned the localized verified routes feature.. I just tried to integrate localized routes into one of our LiveView applications and encountered the following problem.

We have the following LiveViews/folder structure:

article_live/index.ex
article_live/index.hmtl.heex
article_live/show.ex
article_live/show.hmtl.heex

In the router I specified the following routes:

localize do
      live "/#{locale}/article", MyAppWeb.ArticleLive.Index, :index
      live "/#{locale}/article/:id", MyAppWeb.ArticleLive.Show, :show
end

For the index paths the following routes and helper functions are generated (because of our naming convention of the LiveViews):

      index_de_path  GET   /de/article                                  MyAppWeb.ArticleLive.Index :index
      index_en_path  GET   /en/article                                  MyAppWeb.ArticleLive.Index :index

Because I don't want to rename our LiveViews I tried to use the as options of live/4 but the Cldr expects an binary value:

live "/#{locale}/article", MyAppWeb.ArticleLive.Index, :index, as: :article

** (ArgumentError) construction of binary failed: segment 1 of type 'binary': expected a binary but got: :article
    (ex_cldr_routes 0.6.2) lib/cldr/route.ex:622: Cldr.Route.helper_name/3
    (ex_cldr_routes 0.6.2) lib/cldr/route.ex:587: Cldr.Route.localise_helper/3
    (ex_cldr_routes 0.6.2) lib/cldr/route.ex:477: Cldr.Route.do_localize/4
    (ex_cldr_routes 0.6.2) expanding macro: Cldr.Route.localize/2

When I change the option to as: "article" the Phoenix LiveView router expects an atom:

** (ArgumentError) expected live :as to be an atom, got: "article"

    (phoenix_live_view 0.18.15) lib/phoenix_live_view/router.ex:400: anonymous fn/1 in Phoenix.LiveView.Router.validate_live_opts!/1
    (elixir 1.14.3) lib/enum.ex:975: Enum."-each/2-lists^foreach/1-0-"/2
    (phoenix_live_view 0.18.15) lib/phoenix_live_view/router.ex:387: Phoenix.LiveView.Router.validate_live_opts!/1
    (phoenix_live_view 0.18.15) lib/phoenix_live_view/router.ex:359: Phoenix.LiveView.Router.__live__/4
    lib/sourceboat_web/router.ex:38: (module)
    (elixir 1.14.3) lib/kernel/parallel_compiler.ex:340: anonymous fn/5 in Kernel.ParallelCompiler.spawn_workers/7

With the new localized verified routes feature this will no longer be an issue as the route helpers will no longer be needed, but I think it would also be nice to make this configurable. Or maybe we need to revise our naming conventions after all...

What do you think?

kipcole9 commented 1 year ago

Thanks for reporting this. This is the one lib I struggle with motivation on - exactly because the helper code is a complete pain in the .... But that's no excuse, I need to make it work and I need to get verified routes finished up (the plumbing is done but the translation chore isn't).

I will absolutely get this sorted in the next week. Thanks for your patience, especially given how badly I've maintained this one (different to all the other cldr libs).

krns commented 1 year ago

Don't worry, I appreciate that this library exists and all the work that goes into it - as probably do many others!

kipcole9 commented 1 year ago

No worries, I'd much prefer you raise the issue. Its a bug and it needs fixing so I'll aim to get this resolved over the weekend.

elixir-cldr / cldr_routes

Fix configure example in readme #12

Bug Fixes