Skip to content
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

Add .defined_schema method #98

Merged
merged 1 commit into from
May 7, 2018
Merged

Add .defined_schema method #98

merged 1 commit into from
May 7, 2018

Conversation

glaucocustodio
Copy link
Contributor

No description provided.

context 'json schema defined' do
let(:instance) { WithSchema }
it do
expect(instance.defined_schema).to eq({ name: String, age: Integer })

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

# Person.defined_schema
# # => { name: String }
def defined_schema
instance_variable_get('@__surrealist_schema')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we raise UnknownSchemaError or any other error if any schema has been defined?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we should

Copy link
Owner

@nesaulov nesaulov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a spec for the same logic but with custom serializers?

# Person.defined_schema
# # => { name: String }
def defined_schema
instance_variable_get('@__surrealist_schema')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we should

CHANGELOG.md Outdated
# Next (unreleased)

## Added
* `.defined_schema` to return the schema that has been defined with `json_schema` ([@glaucocustodio][]) [#93](https://github.com/nesaulov/surrealist/pull/98)
Copy link
Owner

@nesaulov nesaulov Apr 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add a link to yourself at the end of the file in order for this to work, see line 95

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the url text is wrong - this is PR #98, not #93

describe '.defined_schema' do
context 'json schema defined' do
let(:instance) { WithSchema }
it do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A space between let and it is missing.
Also I would suggest to add a specification to it, like it 'returns the defined json_schema' do


context 'json schema not defined' do
let(:instance) { WithoutSchema }
it do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@glaucocustodio
Copy link
Contributor Author

class CatSerializer < Surrealist::Serializer
  json_schema { { age: Integer, age_group: String } }

  def age_group
    age <= 5 ? 'kitten' : 'cat'
  end
end
class Cat
  include Surrealist
  attr_reader :age

  surrealize_with CatSerializer

  def initialize(age)
    @age = age
  end
end

In this case of custom serializer, should Cat respond to defined_schema as well?

@nesaulov
Copy link
Owner

I think yes, it should. And if there are many serializers available (via tag argument) we should look at the default one I think. Later on we can add possibility to specify tag inside defined_schema, but it's not necessary right now I think

@glaucocustodio
Copy link
Contributor Author

Take a look now please @nesaulov

Copy link
Owner

@nesaulov nesaulov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the url text in changelog, the rest looks good to me

CHANGELOG.md Outdated
# Next (unreleased)

## Added
* `.defined_schema` to return the schema that has been defined with `json_schema` ([@glaucocustodio][]) [#93](https://github.com/nesaulov/surrealist/pull/98)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the url text is wrong - this is PR #98, not #93

@glaucocustodio
Copy link
Contributor Author

Done @nesaulov

Copy link
Owner

@nesaulov nesaulov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @glaucocustodio!

@nesaulov
Copy link
Owner

nesaulov commented May 4, 2018

🤔 there is something wrong with rspec or rom on travis

@nesaulov
Copy link
Owner

nesaulov commented May 5, 2018

The problem was with dry-types and rom-rb compatibility, see rom-rb/rom#494
I have locked dry-types version for now in #101, should be fine now. @glaucocustodio please merge master in your branch.

@nesaulov
Copy link
Owner

nesaulov commented May 7, 2018

Thank you @glaucocustodio! 🎉
Sorry that this took so long

@nesaulov nesaulov merged commit 500250f into nesaulov:master May 7, 2018
@glaucocustodio glaucocustodio deleted the defined-schema branch May 7, 2018 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants