-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds basic functionality for managing attributes with several options #121
Conversation
I think the API looks cleaner, and especially the possibility to filter seems great. Would you say |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good :)
lib/fortnox/api/models/base.rb
Outdated
@@ -30,6 +30,17 @@ def self.new( hash ) | |||
IceNine.deep_freeze( obj ) | |||
end | |||
|
|||
# This filtering logic could be improved since it is currently O(N*M). | |||
def attributes( *options ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add tests for this this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yepp.
lib/fortnox/api/models/base.rb
Outdated
def attributes( *options ) | ||
return self.class.schema if options.nil? | ||
|
||
options = Array(options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe choose a different variable name here instead of mutating options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but on the other hand I'm just ensuring that the argument is an array, I'm not really mutating it. Also I think this is unnecessary, the argument should already be an array from how I get them. I'll verify.
lib/fortnox/api/models/customer.rb
Outdated
@@ -27,7 +27,7 @@ class Customer < Fortnox::API::Model::Base | |||
# ) | |||
|
|||
#Url Direct URL to the record | |||
attribute :url, Types::Nullable::String.with( read_only: true ) | |||
attribute :url, Types::Nullable::String.is( :read_only ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -1,6 +1,25 @@ | |||
require 'dry-struct' | |||
require 'dry-types' | |||
|
|||
module Dry | |||
module Types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should definitely add test this override. We want to verify it's working as intended and we also want to see if it breaks when we update Dry::Types
in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make this a refinement instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great suggestion! Then we only need to test that our Types
is behaving as intended and it will prevent there modifications from leaking too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out you can't refine modules, only classes. I tried a few workarounds but didn't get it to work at all with refinements :/
I'll probably open a PR against dry types and try to get this into the real repo. If not I'll ask @solnic to advice on how to do this refinement in a safe way ...
Right now we do
.with( read_only: true )
to add options to an attribute declaration. This PR allows you to use.is( :read_only )
instead. It also works with several options, so.with( read_only: true, required: true, sortable: true, filterable: true )
becomes.is( :read_only, :required, :sortable, :filterable )
instead.It also adds an
.attributes()
method to our instances where you get all the attributes on the class by default but can give it one or more options to filter the list, likeComments on implementation and strategy for this is welcome, it's just a rough WiP.
When we have this we can easily get a list of sortable and filterable attributes into our models and use that info when the filtering methods are called to verify that only attributes that are actually filter attributes are sent to Fortnox.