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

RFC Support a namespace via config #195

Closed
dantswain opened this issue Jan 25, 2017 · 5 comments
Closed

RFC Support a namespace via config #195

dantswain opened this issue Jan 25, 2017 · 5 comments

Comments

@dantswain
Copy link
Collaborator

Using namespace elixir MyStuff in the .thrift files produces a pile of warnings along the lines of

[WARNING:/some/path/my_struct.thrift:6] No generator named 'elixir' could be found!

whenever running the official thrift compiler to generate code for another language.

This can be disabled by using -nowarn but that suppresses all warnings, which seems like a bad idea.

I think supporting the namespace keyword is fine, but there's something wonky to me about requiring people to opt in to a pile of warnings in their pre-existing workflow. One way to get around this would be to make the namespace keyword optional and support a namespace option in the compile step configuration (see #194 - thrift: [namespace: MyStuff, input_files: ...]).

@jparise
Copy link
Collaborator

jparise commented Jan 26, 2017

I think this is a good suggestion. There's one part of the behavior that I think still need some specificity, though. When a top-level namespace option is configured and we encounter a namespace keyword in a .thrift file, should we:

  1. Use the file's namespace, overriding the top-level compiler option
  2. Use the top-level compiler option, ignoring all file-level namespaces
  3. Append the file-level namespace to the top-level namespace

I think 1 is the more obvious, but I can see arguments for the others.

We could also make the top-level namespace compiler option do fancy things like per-file overrides when e.g. a map or function is given instead of a string, but that's probably more complicated than we actually need.

@dantswain
Copy link
Collaborator Author

I also like 1. I ran into this because I added elixir namespaces to some of our thrift files and now people are asking me about the warnings. It would solve my problem if I could just specify the namespace as an option, and having it overridable at the file level would still give people flexibility.

Another option would be to be able to specify groups of files -

def project do
  thrift: [
    namespace: [Namespace1: ["foo.thrift", "bar.thrift"], Namespace2: ["baz.thrift"]]
  ]
end

That may be overcomplicated though.

@pguillory
Copy link
Collaborator

What if we supported elixir namespaces in comments? So other compilers would ignore it.

namespace cpp whatever
namespace java com.universe.milkyway.earth.whatever
#namespace elixir Whatever

Kind of a hack, but on the other hand #define wasn't the end of C.

@jparise
Copy link
Collaborator

jparise commented Feb 9, 2017

What if we supported elixir namespaces in comments?

That's a creative solution, but after thinking about it a bit, and I'm 👎 on it because:

  1. It makes our parsing rules more complicated, and we'd be setting precedent for supporting other directives using this syntax.
  2. It still leaves a gap for the case where a .thrift file hasn't been modified to specify a namespace, so we'd still need a global default to provide that value.

jparise added a commit that referenced this issue Feb 10, 2017
Up to this point, the only way to specify a schema's namespace was via
the `namespace elixir` Thrift IDL directive. Unfortunately, the Apache
Thrift compiler barks when it encounters "unknown" namespace types. This
also requires all consumers of our library to explicitly add namespace
directives to all of their Thrift IDL files.

This change implements the simplest strategy discussed in #195: a new
top-level `thrift.namespace` configuration option can be set to a global
default namespace. Any schemas without its own `elixir` namespace will
fall back to using this global default value.
@jparise
Copy link
Collaborator

jparise commented Feb 10, 2017

Closed via #225

@jparise jparise closed this as completed Feb 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants