mountetna / magma

Data server with friendly data loaders
GNU General Public License v2.0
5 stars 2 forks source link

Remove identifier type from spec models #124

Closed alimi closed 4 years ago

alimi commented 4 years ago

Magma::Model#identifier sets the type to string so there's no need to set it in the definitions.

https://github.com/mountetna/magma/blob/d6764d31e6e64b9447f6480e6af58cf14c3db0e1/lib/magma/model.rb#L50-L57

Also, update Magma::Model#identifier so opts defaults to an empty hash. Some spec models don't set any options.

graft commented 4 years ago

May we also at this point remove :type here? https://github.com/mountetna/magma/blob/d6764d31e6e64b9447f6480e6af58cf14c3db0e1/lib/magma/attribute.rb#L10

graft commented 4 years ago

Might be tempting to cover some of #125 in here, what do you think?

alimi commented 4 years ago

It's tempting to remove:type from Magma::Attribute.options, but it's still being used. The attribute classes set type on initialization

https://github.com/mountetna/magma/blob/d6764d31e6e64b9447f6480e6af58cf14c3db0e1/lib/magma/attributes/boolean_attribute.rb#L1-L7

and that only works if :type is in Magma::Attribute.options.

https://github.com/mountetna/magma/blob/d6764d31e6e64b9447f6480e6af58cf14c3db0e1/lib/magma/attribute.rb#L126-L128

@type is still being used in Magma::Attribute, and Magma::Migration uses it as _type.

https://github.com/mountetna/magma/blob/d6764d31e6e64b9447f6480e6af58cf14c3db0e1/lib/magma/migration.rb#L79-L89

alimi commented 4 years ago

Maybe we save #125 for another PR. It's tackling a little more than just removing type from some identifier calls 😄

graft commented 4 years ago

:+1: on #125.

Regarding :type, my intent is only to remove it from the options interface, to prevent bizarre things like collection :samples, type: String, which right now seems like it would pass through. We would still ultimately like to be able to set @type for each attribute class, but after removing :type from Magma::Attribute.options, we could do:

class Magma 
   class BooleanAttribute < Attribute 
     def initialize(name, model, opts) 
       @type = TrueClass
       super
     end 
   end 
 end 

Again maybe out of scope here, and perhaps not that important since we are ditching the model interface anyway, and it is not clear what will happen to "opts" when the attribute is configured through the database model.

alimi commented 4 years ago

Gotcha, thanks for clarifying! I removed it in https://github.com/mountetna/magma/pull/124/commits/dedda118a74d9dc041280813b7dd6ec7f57990a1 with a slightly different approach.

graft commented 4 years ago

Looks nice, I like avoiding the initializer - since it is not really hidden, can we change _type to databasetype for clarity? That initial everywhere is like a bunch of rusty nails sticking out.

alimi commented 4 years ago

Good call! Done in https://github.com/mountetna/magma/pull/124/commits/48c98466d202049656668bcee07250650d4f2e57.

graft commented 4 years ago

:ship: