Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File of constants not generating any code, causes compile error #196

Open
dantswain opened this issue Jan 25, 2017 · 4 comments
Open

File of constants not generating any code, causes compile error #196

dantswain opened this issue Jan 25, 2017 · 4 comments
Assignees
Labels

Comments

@dantswain
Copy link
Collaborator

(On thrift_tng)

The compile error is

** (Enum.EmptyError) empty error
    (elixir) lib/enum.ex:1590: Enum.reduce/2
    (mix) lib/mix/utils.ex:80: Mix.Utils.stale_stream/2
    (mix) lib/mix/utils.ex:66: Mix.Utils.stale?/2
    (elixir) lib/enum.ex:787: anonymous fn/3 in Enum.filter/2
    (elixir) lib/enum.ex:1623: Enum."-reduce/3-lists^foldl/2-0-"/3
    (elixir) lib/enum.ex:787: Enum.filter/2
    lib/mix/tasks/compile.thrift.ex:42: Mix.Tasks.Compile.Thrift.run/1
    (mix) lib/mix/task.ex:296: Mix.Task.run_task/3

The error appears to occur in this function in the compile task when targets comes back as an empty list:

  defp stale?(%FileGroup{initial_file: thrift_file} = group, output_dir) do
    targets =
      group
      |> Thrift.Generator.targets
      |> Enum.map(&Path.join(output_dir, &1))
    Mix.Utils.stale?([thrift_file], targets)
  end

The file I'm parsing just contains some const i32 definitions that aren't referenced in the other thrift files anywhere (they get used in applications that use this set of thrift files). If I pass --force to the compiler, it gets past this stage but then my library fails to compile because the constants are undefined.

I'll try to come up with a test case that exercises this and if I see an easy fix I'll make a PR.

@dantswain
Copy link
Collaborator Author

It actually doesn't look like const values are generating any code at all?

@dantswain
Copy link
Collaborator Author

I have a POC solution for this. The approach I took was, given a file "foo.thrift" with a non-empty set of constants, generate a module called FooConstants that defines a function for each constant that returns the value of the constant. Is this a reasonable approach? A couple alternatives I thought of:

  • Generate Foo.Constants instead of FooConstants (FooConstants seems consistent with the way other thrift libs work and we don't nest any other modules aside from namespacing)
  • Generate macros instead of functions (this could get hairy with quotes/unquotes and compilation order?)

@pguillory
Copy link
Collaborator

It definitely should generate something for constants -- only reason it doesn't is that we hadn't gotten to it.

Naming it is tricky. Using the filename as you suggested, foo.thrift and subdir/foo.thrift would both generate a FooConstants module. Using the namespace, same thing, multiple thrift files can contain the same namespace. I think I favor generating a module based on the namespace though because we have more control over it. We have a bunch of thrift files that we can't easily move around or rename, but all these other systems depend on them and they include each other, but we can add elixir namespaces to them to control what gets generated.

I know you have the separate issue with Apache raising errors about unrecognized namespaces. We need to fix that somehow.

Regarding macros vs functions, I liked macros for enums because they're so often used for matching. For constants, maybe it doesn't make as much difference. You can always put the function call in a variable or module attribute or unquote if you really need to match it, but it seems like it would happen less often. What do you all think?

@jparise
Copy link
Collaborator

jparise commented Feb 16, 2017

I'm seeing some of the issues that @pguillory mentions in the previous comment with the current approach to generating constants.

Start with a file named userservice.thrift that includes some constants and a service UserService definition. The generator will produce a file named userservice.ex (defmodule Userservice) containing the constants and a file named user_service.ex (defmodule UserService) containing the service definition.

I'm not quite sure what should change, but it's a bit confusing, and I suppose we could run into problems on case-insensitive file systems.

I suppose we could look at merging the constants into user_service.ex (defmodule UserService) because it's the most similar name. But that's more of a coincidence with the service name matching the name of the .thrift file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants