molybdenum-99 / reality

Comprehensive data proxy to knowledge about real world
MIT License
817 stars 43 forks source link

Entity#non_existent_attribute should raise NoMethodError #54

Closed ptolemybarnes closed 8 years ago

ptolemybarnes commented 8 years ago

Things like Entity("Paris").starsign should raise NoMethodError, rather than just returning nil. This would be more like how normal Ruby objects behave.

zverok commented 8 years ago

That's hard! Let's look at this example:

list = Reality::List.new('Kharkiv', 'Kyiv', 'Odessa', 'Poltava', 'Lviv')
list.load!
list.describe
# -------------------------
# #<Reality::List(5 items)>
# -------------------------
#  keys: adm_divisions (2), area (5), awards (1), coord (5), country (5), created_at (1), elevation (4), located_in (4), long_name (3), neighbours (1), official_website (4), population (5), population_metro (3), tz_offset (3)
# types: city (5)

# Note that not all of cities have same set of keys
# Now, if you have some mass-processing code, you can, for example:
list.select(&:elevation)
# => #<Reality::List[Kharkiv, Kiev, Odessa, Lviv]>
# or
list.map(&:elevation).compact
# or
list.map(&:elevation).map(&:to_s)
# => ["152m", "179m", "40m", "", "296m"]

# Now, with your change:
list.select{|l| l.respond_to?(:elevation)}
# or
list.select{|l| l.methods.include?(:elevation)}

I think, current behavior is what POLS requires. As we have no concrete "schema" for city, we can't pre-decide which method it has and which has not. So, we just assume it has all of them!

For experiments and demos (important Reality targets) it is the most clear way to express intentions.

zverok commented 8 years ago

BTW, "normal Ruby objects" typically have same set of methods/behaviors if they are of same class, which is not true for our entities. So, I prefer to think of them as of something closer to OpenStruct or Hashie::Mash instances.

ptolemybarnes commented 8 years ago

Fair enough. I haven't used OpenStruct before. I'd argue that once you load an entity, e.g Reality::Entity.new('Kharkiv'), it has acquired a schema of sorts? I can see that, in the case of lists, you might want your enumerable methods to rescue NoMethodError and return nil instead.

Also I just had a look at List. Why not include Enumerable, rather than subclass Array? Sorry I have so many questions! This project is super cool.

zverok commented 8 years ago

Thank you!

I'd argue that once you load an entity, e.g Reality::Entity.new('Kharkiv'), it has acquired a schema of sorts?

For singular entity, yes. If you investigate two entities with similar type, but different set of known properties, it becames vague. Imagine somebody giving lecture/presentation and does some thing like:

k = E('Kharkiv')
# => #<Reality::Entity(Kharkiv):city> 
k.describe
# --------------------------------
# #<Reality::Entity(Kharkiv):city>
# --------------------------------
#     adm_divisions: #<Reality::List[Ordzhonikidze District?, Shevchenkivskyi District?, Kiev District?, Comintern District?, Moscow District?, Zhovtnevy District?, Frunze District?, Chervonozavodsky District?, Lenin District?]>
#              area: #<Reality::Measure(350 km²)>
#             coord: #<Reality::Geo::Coord(50°0′0″N,36°13′45″E)>
#           country: #<Reality::Entity?(Ukraine)>
#        created_at: #<Date: 1654-01-01>
#         elevation: #<Reality::Measure(152 m)>
#        located_in: #<Reality::Entity?(Kharkiv Oblast)>
#         long_name: "Kharkiv (Харків)\nKharkov (Харьков)"
#  official_website: "http://www.city.kharkov.ua/"
#        population: #<Reality::Measure(1,430,885 person)>
#  population_metro: #<Reality::Measure(1,732,400 person)>
#         tz_offset: #<Reality::TZOffset(UTC+02:00)>
# 

k.population
# => #<Reality::Measure(1,430,885 person)> 
k.elevation
#  => #<Reality::Measure(152 m)> 

# OK, says lector to students, and it works always! For ex...
p = E('Poltava')
# => #<Reality::Entity(Poltava):city> 
p.population
# => #<Reality::Measure(298,652 person)>
p.elevation
# => nil -- that looks "ok, we don't know elevation value for Poltava"

#...while this:
p.elevation
# => NoMethodError Entity(Poltava) has no `elevation` method + long backtrace
# ...looks like "something broken" in our data source

Of course, it has its drawbacks. If you write k.elvation -- note the misprinted "elvation" -- it will silently eat this, returning nil instead of notifying you of your error... But, currently, its the most reasonable approach I can think of

Also I just had a look at List. Why not include Enumerable, rather than subclass Array?

Just because I'm a huge believer of Do The Simplest Thing That Could Possibly Work principle. For this case (in my head) "the simplest thing" was just do some special kind of Array, which is totally like your normal Array, but sometimes it is better :)