-
Notifications
You must be signed in to change notification settings - Fork 64
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
2.0.0 SDK #113
2.0.0 SDK #113
Conversation
5a5a1f5
to
7d2d7d7
Compare
7d2d7d7
to
7df6545
Compare
7df6545
to
20f3d9d
Compare
4669ea6
to
c7eaa68
Compare
## Changed | ||
|
||
* The removal of the Client and Request objects from the Resource objects, means that for example: Link#resolve and Array#next_page now require the client as a parameter. | ||
* Client#entry now uses /entries?sys.id=ENTRY_ID instead of /entries/ENTRY_ID to properly resolve includes. |
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.
👍
# Another possibility to create customized resources is to just inherit from an | ||
# existing one: | ||
# To then have it mapped automatically from the client, | ||
# upon client instanciation, you set the :entry_mapping for your ContentType. |
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.
Nit: instantiation, not instanciation
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.
Fixed
def localized?(value) | ||
return false unless value.is_a? ::Hash | ||
value.keys.any? { |possible_locale| Contentful::Constants::KNOWN_LOCALES.include?(possible_locale) } | ||
end | ||
|
||
# Checks if value is a links |
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.
nit: Checks if value is a link
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.
Fixed
property :country, Contentful::Locale | ||
# You can define your own custom classes that inherit from Contentful::Entry. | ||
# This allows you to define custom behaviour, for example, in this case, we want | ||
# the :country field to act as a Contentful::Locale |
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.
👍
else | ||
false | ||
end | ||
def next_page(client = nil) |
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.
Cool, I see this is the thing you were referring to in the changelog. 👍
# @private | ||
def inspect | ||
"<#{repr_name} total=#{total} skip=#{skip} limit=#{limit}>" | ||
end | ||
|
||
# Simplifies pagination | ||
# | ||
# @return [Contentful::Array, false] |
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.
is false
the object type or is it something like Bool
? just curious i'm used to return comments showing the type or just the parameter name
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.
There is no Boolean
class in Ruby, there are TrueClass
and FalseClass
, of which true
and false
are singletons of.
end | ||
|
||
# Issues the request that was made to fetch this response again. | ||
# Only works for top-level resources |
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.
Mind elaborating on what you mean by top-level
here?
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.
Fixed
|
||
private | ||
|
||
def create_files! |
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.
I guess I don't understand this create_files
method (or at least it's underlying purpose). So you are essentially storing a JSON file that has been localized?
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.
It wraps the Asset's File Raw JSON into File Objects for every locale
|
||
def initialize(item, configuration = {}, _localized = false, _includes = [], depth = 0) | ||
@raw = item | ||
@default_locale = configuration[:default_locale] |
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.
what does this configuration object/dictionary look like?
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.
It's a copy of the configuration object sent to the resource builder, can be seen here: https://github.com/contentful/contentful.rb/pull/113/files#diff-ccad056e91bbffbe74262ffa2994a804R277
It's non-essential and internal, if you want to create your own instances it's not required to send anything there.
message += "Retrying - Retries left: #{retries_left}" | ||
message += "- Time until reset (seconds): #{reset_time}" | ||
message | ||
end |
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.
👍
return value if type.nil? | ||
|
||
options = {} | ||
options[:coercion_class] = KNOWN_TYPES[items.type] unless items.nil? |
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.
I think a comment about the usage of these options would be helpful. I had to scroll back up to the coercion file to understand that they are basically for Array's
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.
This actually currently only works for Entries and Assets, Arrays have been purposely removed.
'Asset' => Asset, | ||
'Array' => :array_or_sync_page, | ||
'Array' => Array, |
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.
👍
@default_locale = default_locale | ||
@resource_mapping = default_resource_mapping.merge(resource_mapping) | ||
@entry_mapping = default_entry_mapping.merge(entry_mapping) | ||
def initialize(json, configuration = {}, localized = false, depth = 0, endpoint = nil) |
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.
Again, I'm a bit confused by the usage of the configuration
. Did I miss a comment about it somewhere?
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.
The configuration object here is just to reduce the amount of things sent around, also, the same configuration options are required to be sent around to other places, so instead of having a thousand parameters, having a hash of configuration options seemed more reasonable.
Also, these options are internal, and non-required if someone wants to create a custom class
Fixes #109
Fixes #104
Obsoletes #84
This PR is intended to remove all legacy code from the SDK.
The API grew along the years considerably, and many hacks were put in place in order to keep up with it.
Breaking changes introduced:
Link#resolve
andArray#next_page
now require the client as a parameter.Client#entry
now uses/entries?sys.id=ENTRY_ID
instead of/entries/ENTRY_ID
to properly resolve includes.ResourceBuilder
DynamicEntry
andResource
ContentTypeCache
outside of the client into it's own classBaseResource
andFieldsResource
classes to handle common resource attributes and fields related attributes respectivelyContentType
, eachField
knows which coercion should be applied depending onField#type
#inspect
now provides a clearer and better output, without all the noise that was previously thereCustomResource
was removed, now subclasses ofEntry
should be used instead.