search

mountetna / magma

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

Change attribute definitions #118

Closed notmarkmiranda closed 4 years ago

notmarkmiranda commented 4 years ago

113

This will start work on the linked ticket. It will make the #attribute method private within the model class and allow for attribute creation by using data_type :attribute_name where the allowed data_types are string, integer, boolean, and date_time.

notmarkmiranda commented 4 years ago

I think because you used class << self the private actually works. I threw a pry in one of the tests and attempted to call Magma::Model.attribute and get a NoMethodError.

image
graft commented 4 years ago

While that works if you stick a pry in class Labors::Aspect e.g. you are inside the class and can access #attribute.

On Tue, Apr 7, 2020, 5:53 PM Mark Miranda notifications@github.com wrote:

I think because you used class << self the private actually works. I threw a pry in one of the tests and attempted to call Magma::Model.attribute and get a NoMethodError.

[image: image] https://user-images.githubusercontent.com/15822772/78732819-f4aa8c00-7900-11ea-9af1-bb2e3c4d36a1.png

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/mountetna/magma/pull/118#issuecomment-610691755, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACHPA7TJDICEL5ZCYHRRS3RLPDHRANCNFSM4MDOCVUQ .

graft commented 4 years ago

A few observations:

1) Magma::Model._attribute remains and continues to be used by .collection, .matrix, etc. - since _attribute does the same work as set_attribute this interface confusion ought to be resolved. This division is also reflected in the list of attribute classes used to define #integer, etc. in Magma::Model, which includes the previously classless attributes but excludes the existing attribute classes (CollectionAttribute, MatrixAttribute, etc.).

2) There is a 'json' attribute that i don't think should exist. While in the future we might expose a generic json attribute, for the moment json columns are used only in specific ways (to hold the data for the match type or the matrix type) and should not be exposed as an attribute class.

3) The new use of IntegerAttribute etc. is not reflected in case statements like the one in #attribute_child in lib/magma/query/predicate/record.rb, which switches on attribute.type. The code still works because IntegerAttribute inherits from Magma::Attribute and exposes #type, so the method continues to function. Probably, however, we should hide Magma::Attribute#type since it does not need to be exposed and resolve this case statement to use the attribute class rather than the type.

notmarkmiranda commented 4 years ago

Labors::Prize and Labors::Olympians tests have json: attributes. If I remove that, what type should that be stored as?

graft commented 4 years ago

Could either infer the type from self.class.name or put it all in a setup method called in the initializer and overriden by imageattribute.

On Tue, Apr 14, 2020, 2:15 PM Mark Miranda notifications@github.com wrote:

@notmarkmiranda commented on this pull request.

In lib/magma/attributes/file.rb https://github.com/mountetna/magma/pull/118#discussion_r408440081:

   @type = String
  • Magma.instance.storage.setup_uploader(model, name, :file)

I'm not sure how to fix this? I can add an initialize method to ImageAttribute, but there's nothing stop this from being run in the ImageAttribute#new and then again in FileAttribute#new when super is called?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/mountetna/magma/pull/118#discussion_r408440081, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACHPA4UJ4R6DTMGAWZKIATRMTG7RANCNFSM4MDOCVUQ .