Skip to content

Conversation

TylerLeonhardt
Copy link
Contributor

@TylerLeonhardt TylerLeonhardt commented Apr 19, 2017

defmodule MyApp.SomeModel do
  use ElasticSync.Schema,
    index: "some_model",
    type: "blah",
    config: {MyApp.SomeModel, :index_config}

  def index_config do
    %{settings: %{}, mappings: %{}}
  end
end

you can now define settings and mappings in a function that will get plumbed down to the creation of the elastic search index

As talked about in #5

@coveralls
Copy link

coveralls commented Apr 19, 2017

Coverage Status

Coverage increased (+0.9%) to 80.531% when pulling 54f2a05 on tylerl0706:master into 1f6fc66 on promptworks:master.

names
|> API.index
|> HTTP.put
|> HTTP.put(Tirexs.get_uri_env(), config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the Tirexs.get_uri_env necessary?


def transition(name, fun) do
transition(name, get_new_alias_name(name), fun)
def transition(name, fun, config \\ []) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be consistent with the type we're expecting for config. Is it a keyword list or a map? Technically tirexs will accept both, but I'd actually prefer we consistently default to a map.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config should be the second arg. the function should be the last.

i don't think we need to provide a default here. especially since I plan to make all of these functions take a struct later anyway.

{mod, fun} = Keyword.get(opts, :config, {ElasticSync.Schema, :default_config})

validate_index_name!(index)
validate_config!(mod, fun)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it isn't a required option, and since developers will just be entering raw values (not some computed value), I don't think we need to check for nil.

@rzane
Copy link
Contributor

rzane commented Apr 19, 2017

This looks great. I just have a few minor suggestions. Then, once CI is passing, this should be good to go!

@coveralls
Copy link

coveralls commented Apr 19, 2017

Coverage Status

Coverage increased (+2.4%) to 81.982% when pulling ef21a29 on tylerl0706:master into 1f6fc66 on promptworks:master.

@coveralls
Copy link

coveralls commented Apr 19, 2017

Coverage Status

Coverage increased (+0.6%) to 80.18% when pulling d978c05 on tylerl0706:master into 1f6fc66 on promptworks:master.

@coveralls
Copy link

coveralls commented Apr 19, 2017

Coverage Status

Coverage increased (+0.6%) to 80.18% when pulling 8b69cdb on tylerl0706:master into 1f6fc66 on promptworks:master.

@coveralls
Copy link

coveralls commented Apr 19, 2017

Coverage Status

Coverage increased (+0.6%) to 80.18% when pulling 8f1a25b on tylerl0706:master into 1f6fc66 on promptworks:master.

@coveralls
Copy link

coveralls commented Apr 19, 2017

Coverage Status

Coverage increased (+0.6%) to 80.18% when pulling 7a5b7f4 on tylerl0706:master into 1f6fc66 on promptworks:master.

@rzane
Copy link
Contributor

rzane commented Apr 19, 2017

I'm good with this as soon as CI passes 👍

@coveralls
Copy link

coveralls commented Apr 19, 2017

Coverage Status

Coverage increased (+0.4%) to 80.0% when pulling d99fbd2 on tylerl0706:master into 1f6fc66 on promptworks:master.

@coveralls
Copy link

coveralls commented Apr 22, 2017

Coverage Status

Coverage decreased (-0.9%) to 78.761% when pulling 561681d on tylerl0706:master into 1f6fc66 on promptworks:master.

@coveralls
Copy link

coveralls commented Apr 22, 2017

Coverage Status

Coverage increased (+0.9%) to 80.531% when pulling 0da3f89 on tylerl0706:master into 1f6fc66 on promptworks:master.

@TylerLeonhardt
Copy link
Contributor Author

At last!

@rzane rzane merged commit 4483021 into tweag:master Apr 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants