elixir-inspector / ua_inspector

User agent parser library
Apache License 2.0
127 stars 23 forks source link

Database download mix task name #1

Closed koudelka closed 9 years ago

koudelka commented 9 years ago

Slight correction to the database download task name, mix can't find the task otherwise.

mneudert commented 9 years ago

The problem is that this change will require an elixir version of 1.0.3 or higher. Elixir 1.0.2 or lower requires this "underscore module name" or the task will not be available.

Perhaps some delegate-magic will work:

defmodule Mix.Tasks.UAInspector.Databases.Download do
  defdelegate run(args), to: Mix.Tasks.Ua_inspector.Databases.Download
end

(or better the other way round so the delegate can be easily deleted later...)

Will test it out later/tomorrow if you don't find a better solution until then :D

koudelka commented 9 years ago

Ah, I had no idea that was an Elixir 1.0.2 thing, what a luxury to run the latest point release. :)

That inverted delegate sounds good to me, rather than doing manual version checks or something.

mneudert commented 9 years ago

Just one sort of nit-picky change and i will merge it right away:

Please add "@moduledoc false" to the delegate download task so it won't show up in the generated docs.

Otherwise: :shipit:

koudelka commented 9 years ago

All set. Not sure why the coverage went down, though.

mneudert commented 9 years ago

The delegate line is treated as a source line, so it gets included in the coverage. As the test correctly uses the "new" naming it is not called and therefore treated as uncovered. Nothing to worry about :)

koudelka commented 9 years ago

It seems pretty angry under 1.0.3 still (0508494).

$ mix ua_inspector.databases.download

13:52:06.126 [error] Loading of /Users/mikes/active_prospect/trustedform-api/deps/ua_inspector/_build/dev/lib/ua_inspector/ebin/Elixir.Mix.Tasks.UaInspector.Databases.Download.beam failed: :badfile

** (Mix) The task ua_inspector.databases.download could not be found

13:52:06.126 [error] beam/beam_load.c(1250): Error loading module 'Elixir.Mix.Tasks.UaInspector.Databases.Download':
  module name in object code is Elixir.Mix.Tasks.UAInspector.Databases.Download
mneudert commented 9 years ago

Hmpf. Then it is finally time to get version 1.0.3 up and running to find a viable solution to that while still being compatible with 1.0.2.

Worst case will be another delegate task with a "third" naming. but at least there will be something that works :D (and it should not take that long to find...)

koudelka commented 9 years ago

Guessing you have something in production running 1.0.2? :)

mneudert commented 9 years ago

Nothing that would require anything like this weird task naming stuff.

It is more of a usability issue. The task naming in mix was a change called a bugfix and I totally agree with calling it a bugfix. However it broke some stuff with a seriously bad timing.

For example Travis has a default Elixir of 1.0.2 at the moment, resulting in the usage of the old task naming. These little "mix task not found" errors are more than easily misunderstood for what seems like hours if a library requires the fixed naming. Just because the patch release of the language changed. To fix that issue you could just use a specific Elixir version on Travis instead of the default. Resulting in more or less permanently using that version until one manually switches back to the default.

And I just do not like to push that kind of "work" to someone who just wants to use a library. As I would definitely look to get that fixed in the library itself ^^

mneudert commented 9 years ago

Tested it locally with both 1.0.2 and 1.0.3 and all tasks were callable with the correct name.

Actually had to mess with the compilation to generate one of two delegate tasks. Not the cleanest way, but at least it should work ;)

koudelka commented 9 years ago

What do you think of defining the tasks like this? It's a little less code.

module_name = \
  if Version.match?(System.version, ">= 1.0.3") do
    Mix.Tasks.UaInspector.Databases.Download
  else
    Mix.Tasks.Ua_inspector.Databases.Download
  end

contents = quote do
  @moduledoc false
  @shortdoc  "Downloads parser databases"

  use Mix.Task

  defdelegate run(args), to: Mix.Tasks.UAInspector.Databases.Download
end

Module.create(module_name, contents, Macro.Env.location(__ENV__))
mneudert commented 9 years ago

Less code, but it also looks more complicated. Using just a quoted variable and defmodule unquote(task) do (or even a simple variable without any quoting) might also do the trick.

But as long as it works I am totally fine with having a dozen lines more code. Makes it easier to fit in a comment why that stuff was necessary in the first place.

koudelka commented 9 years ago

okie. :)