feature: type_from_name

[mikkoh]

This PR just adds a function type_from_name onto schema which can be used to query for a type based on the name of the type.

[dylanahsmith]

Making this convenient to call a lot could also make a code generator slow. Would it make sense to build and memoize a hash mapping name to type instead?

[mikkoh]

Or would it make sense to initialize a look up table in the constructor?

[dylanahsmith]

that just means it would be constructed without being used in the typical case of just iterating over the types

[mikkoh]

Yes but memoizing when type_from_name is used would mean you only have a performance increase if you're accessing the same types often.

We could create a look up table the first time type_from_name is used which would be the best of both worlds.

[dylanahsmith]

oh, that's true. Something like

@types_by_name ||= {}
@types_by_name[type_name] ||= @types.find { |type| type.name == type_name }

[mikkoh]

I was thinking build the entire look up table when it's type_from_name is used. Because again this would only improve if you continued to access the same type over and over again.

Might improve for something like Shop where you might access it multiple times and it's just under root. But for far off leaf nodes it's unlikely this would bring an improvement.

But maybe this is just splitting hairs. I can implement the way you suggest above. I do not feel strongly about this until it becomes an issue.

[dylanahsmith]

Right, that code example doesn't make sure. Actually, it sounds like you are describing what I said originally: "build and memoize a hash mapping name to type instead". E.g. have a types_by_name method that is memoized, then you could call types_by_name.fetch(type_name) for fast lookups.

[mikkoh]

Ok cool I think we're on the same page. When I read "build and memoize a hash mapping name to type instead" I imagined it being like the code example you posted. HAH (feels like this conversation just went in one big circle)

[mikkoh]

@dylanahsmith I've updated the code. Let me know if there's a better way to do it than I implemented.

[dylanahsmith]

It is usually better to use .fetch if we assume then key exists, since it gives a better error message. Maybe it would be better to just expose the hash and let the caller use the appropriate method to access the type (e.g. key?, [] or fetch).

[mikkoh]

@dylanahsmith I've just exposed types_by_name.

[swalkinshaw]

I had misunderstood your question before, I was suggesting exposing a type method:

def type(name)
  types_by_name.fetch(name)
end

Just like what exists in the tests

[mikkoh]

I think @dylanahsmith had some good comments here #4 (comment)
It's nice if a hash is exposed because you have all the extra functionality that hashes provide.

As for naming I'm ok calling it whatever.

[swalkinshaw]

The hash can still be exposed. I just think the method adds a nicer API.

[dylanahsmith]

A previous iteration of this method used would return nil if the type isn't found and now you are proposing the convenience method to raise a KeyError if the type isn't found. I don't think it is clear what the behaviour should be for this convenience method and just using the hash will make it clearer what the behaviour is if the type isn't found.

[dylanahsmith]

A previous iteration of this method used would return nil if the type isn't found and now you are proposing the convenience method to raise a KeyError if the type isn't found. I don't think it is clear what the behaviour should be for this convenience method and just using the hash will make it clearer what the behaviour is if the type isn't found.