Open Blacksmoke16 opened 4 years ago
Any thoughts on this @robacarp @drujensen ?
I’m ... still, I guess... not really warm to the idea of annotations as a mechanism for general development.
More on topic, fixing the inheritance problem is certainly important, but I think it’ll need more than just Validations, it’ll probably require pulling out of depending on hooks entirely... both finished
and inherited
I think it’s important for Granite to ship with validations built in, but I agree that a substantial refactor is needed to get something which is scaleable in its implementation.
so I am wondering if @Blacksmoke16 can do his magic and support both the assert
gem and then add macros to inject the annotation. This is how the mapping is working. That way you can use the annotation if you want or you can use the macros that wrap them. Can this be done?
but I think it’ll need more than just Validations, it’ll probably require pulling out of depending on hooks entirely... both
finished
andinherited
We don't have any finished
macro hooks left. The only inherited
hooks left are for validations, callbacks, and base. However with the direction it seems we're going in #370, we probably won't need the macro inherited
in base anymore anyway.
I think it’s important for Granite to ship with validations built in
support both the
assert
gem and then add macros to inject the annotation
I think ideally the interface that granite uses should be generic enough to allow for using whatever you want to use, be that assert
, the built in Granite validations, or another shard; assuming it implements the same interface.
I'm pretty sure assert
would already just inherently work since the current implementation uses valid?
which it would override.
But, maybe? The main question would be if you do like
@[Column]
property email : String
@[Assert::Email]
@email : String
where the ivar would have each annotation applied. However, IMO it would be easier to just tell people to add the annotation.
@[Assert::Email]
@[Assert::NotBlank]
column email : String
vs
column email : String
assert Email, :email
assert NotBlank, :email
It also would be less than ideal that granite would have a custom wrapper to a third party shard.
I prefer something a little more rails like:
column email : String
validate :email, format: email, presence: true, allow_blank: false
Which would call the
.exists?
method (#343) on the given type and would only be valid if there is a user with the value ofauthor_id
; which would be useful when, for example, creating a post from form data or JSON.
This is a referential integrity constraint and that is already available in mysql, postgres and sqlite as foreign key constraint.
@drujensen I prefer something a little more rails like too and would prefer something that didn't rely on annotations.
@damianham It looks like crystal is embracing annotations so not sure we have many options. The inherited
or finished
hooks cause other issues like you can't inherit from a Granite Model and they seem to be frowned upon.
I was just hoping to put a little lipstick on the pig.
@damianham
This is a referential integrity constraint and that is already available in mysql, postgres and sqlite as foreign key constraint.
It was more so meant to be an example of how the shard could be easily extended to support custom assertions. The point I was trying to show was related to https://github.com/amberframework/amber/issues/698. I.e. it would allow you to share validations of when you go to save a model AND when validating input from a user; say when deserializing a JSON POST body, or when the form data is submitted.
Currently Granite and Amber use two separate validations systems when it could all be tied into one.
I prefer something a little more rails like too and would prefer something that didn't rely on annotations.
It looks like crystal is embracing annotations so not sure we have many options.
Again, I'm not trying to say use my library because I said so
. The options are:
@Blacksmoke16 Just to clarify, I wasn't calling your Assert
library a pig, I was calling the annotations syntax a pig. I think your code is absolutely beautiful and would love it if we could leverage the Assert
shard.
Option 2 is where we want to go. Just wondering if we can do it without forcing the user to use the annotation syntax.
Just to clarify, I wasn't calling your
Assert
library a pig
I know, all good :P...oink.
Just wondering if we can do it without forcing the user to use the annotation syntax.
I'm sure it's possible, I just don't think it would be trivial. If someone wants to take a stab at it and gets something working id be happy to review it.
See https://github.com/eliasjpr/schema as a possible approach that would not require annotations
The current implementation of model validations is less than ideal. I.e. #123, #238. As I see it we have two options:
Based on the start of some of the conversation around https://github.com/amberframework/amber/issues/698 a possible solution would be to use ~https://github.com/Blacksmoke16/assert~ https://github.com/athena-framework/validator as an optional/recommended dependency. Granite itself could extend
assert
in order to provide built in reusable assertions specific to data model validations, for example:Which would call the
.exists?
method (#343) on the given type and would only be valid if there is a user with the value ofauthor_id
; which would be useful when, for example, creating a post from form data or JSON.The other benefit would be enabling inheritance to be used. I would have to do some testing to see how it actually would work (I imagine there will be some work required to make MTI/STI possible), but would at least prevent compile errors until then.
The main con is of course it would break existing validations and would prevent using Amber with ORMs from using
assert
assertions, if the other ORM does not support applying annotations to their columns.Thoughts?