Skip to content

Discussion: definitions name convention #519

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

Open
JanStevens opened this issue Oct 13, 2016 · 6 comments
Open

Discussion: definitions name convention #519

JanStevens opened this issue Oct 13, 2016 · 6 comments

Comments

@JanStevens
Copy link

In light of the discussion in #515

My proposal is to introduce less magic/assumptions and be more clear about the naming of the definitions.

def model_name(entity)
  if entity.respond_to?(:entity_name)
    entity.entity_name
  else
    entity.name
  end
end
@dblock
Copy link
Member

dblock commented Oct 13, 2016

@JanStevens you should turn this into a pull request with tests, so we can see what it would break

@LeFnord
Copy link
Member

LeFnord commented Oct 22, 2016

@dblock … think it is better to talk about first, so the solution is based on more then one meaning 😉

@JanStevens … first thanks for starting it

think we should start by talking about: "what would be expected by a third party dev from a API documentation?"

the dev always have the route:

/api/<namespace>/<resource>

should the name of the response object be:

{
  "<Resource>": {
    ""
  }
}

or:

{
  "<NamespaceResource>": {
    ""
  }
}

next case

/api/v1/<namespace>/<resourceA>/:keyA/<resourceB>/:keyB

how should this be translated?

@dblock
Copy link
Member

dblock commented Oct 23, 2016

@LeFnord is the boss, I wouldn't listen to me :)

I tend to approach these problems in a very brute-force way and use tests as a way to learn about practical implications and expand the understanding of what a change will impact.

@JanStevens
Copy link
Author

@LeFnord The namespaced resource might show too much information about the internal workings of the API endpoint. (And might become troublesome long for no good reason).

But again if we start to assume the name from the class name then we must take care that multiple class names don't clash together, some examples:

  • Public::Tiger && Internal::Tiger both reference Tiger but are different. Public shows only limited data and internal shows full data
  • ZooEngine::V1::API::Tiger if I give this to an internal dev then great he/she will easily find the name of the related serializer / entity / whatever. For external dev I probably only want to show Tiger

@LeFnord
Copy link
Member

LeFnord commented Oct 26, 2016

Hi @JanStevens, yeap the constraints are now quite clear, but for me the question comes up, how could it be achived in one API?

maybe something like this:

  • take the first name, in your case Tiger,
  • if it exist more then once, concat it with the name before (Internal, Public)
  • and so on

could this be a step in this direction?

@johnvuko
Copy link

johnvuko commented Nov 28, 2016

Hi,
in my project I have quite a few models and so I use a lot the namespacing and I find this PR very useful.
I want to have all my entities having their name with the namespace, so instead of defining a method entity_name in each model I would prefer to just inherit from a class, but in the code https://github.com/ruby-grape/grape-swagger/blob/master/lib/grape-swagger/doc_methods/data_type.rb#L44
you did

        def parse_entity_name(model)
          if model.methods(false).include?(:entity_name)
            model.entity_name
          elsif model.to_s.end_with?('::Entity', '::Entities')
            model.to_s.split('::')[-2]
          elsif model.respond_to?(:name)
            model.name.demodulize.camelize
          else
            model.to_s.split('::').last
          end
        end

If you could just change model.methods(false).include?(:entity_name) by model.methods.include?(:entity_name) it would be very nice.
I think this is more natural and more logic and it avoid to defining the entity_name everywhere like in my case.

By the way, I think showing the full name is a better behaviour than just the name. Having an internal and external entity seem a little strange, (for what I understand) entities are used for expose data through an API so they are not supposed to be private / internal and it's seems to be a lot more improbable than having two classes with the same name in different namespace.

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

4 participants