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

Outside Controller Documentation Use #1083

Closed
wants to merge 10 commits into from
Closed

Outside Controller Documentation Use #1083

wants to merge 10 commits into from

Conversation

hiimtaylorjones
Copy link
Contributor

Fulfilling #1036.

@hiimtaylorjones
Copy link
Contributor Author

Do we want to add a step of saying serializer.to_json for SerializableResource.serialize?

# Optional options parameters
options = {}

# Create the serializer!
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's better to avoid punctuation here.

@hiimtaylorjones
Copy link
Contributor Author

Fixed.

serializer = ActiveModel::SerializableResource.serialize(post, options)
```

This will return an instance of the SerialzableResource class that will allow you to serialze your model.
Copy link
Contributor

Choose a reason for hiding this comment

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

As a reader, I'm left with an ActiveModel::SerializableResource although I was expecting information to make an actual json string.
Edit: You mentioned that before. I think it is definitely worth adding the final step here.

@hiimtaylorjones
Copy link
Contributor Author

Definitely. The initial issue referenced that step as well.

model_json = serializer.as_json
```

This will return an instance of the SerialzableResource class that will allow you to serialze your model.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is a bit awkward here now that you added the final step 😄

@hiimtaylorjones
Copy link
Contributor Author

True that. Deleted the redundant lines.

@@ -13,6 +13,8 @@ This is the documentation of AMS, it's focused on the **0.10.x version.**

- [How to add root key](howto/add_root_key.md)

- [Filtering Assocations](howto/filter_associations.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this related to the current PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not! Good catch there!

@hiimtaylorjones
Copy link
Contributor Author

Updated a few things. Let me know if I missed something!

You could also retrieve the serializer via:

```ruby
ActiveModel::SerializableResource.serialize(post, options).serializer
Copy link
Member

Choose a reason for hiding this comment

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

Due to likely changes, I think we'll want ActiveModel::SerializableResource.new(post, options).serializer

Copy link
Member

Choose a reason for hiding this comment

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

cc @beauby

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes me very happy.

Copy link
Member

Choose a reason for hiding this comment

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

@CodedBeardedSignedTaylor indeed if you are not passing a block you also don't need to use .serialize and can go straight with ActiveModel::SerializableResource.new(post, options).serializer

Copy link
Member

Choose a reason for hiding this comment

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

I think we're going to soon be removing the .serialize method, since it turns out it is more confusing than useful.

so, like @joaomdmoura wrote serializable_resource = ActiveModel::SerializableResource.new(post, options).serializer

(when I wrote the method, I thought the class would be re-used for deserialization..)

@bf4
Copy link
Member

bf4 commented Aug 31, 2015

👍

@bf4
Copy link
Member

bf4 commented Aug 31, 2015

(You might want to rebase and force push...)

@hiimtaylorjones
Copy link
Contributor Author

True that.

@hiimtaylorjones
Copy link
Contributor Author

Did somewhat of a clumsy rebase, but I believe it should be up to date now!

@hiimtaylorjones
Copy link
Contributor Author

Hmmmm. Build is failing for whatever reason. Looking into it now.

@joaomdmoura
Copy link
Member

This is nice, something we're really looking forward to have, can you check the last comment @CodedBeardedSignedTaylor and squash the commits so I can merge it 😄 ?

@joaomdmoura joaomdmoura self-assigned this Sep 8, 2015
@joaomdmoura joaomdmoura added this to the 0.10 milestone Sep 8, 2015

### Retrieving a Resource's Active Model Serializer

If you want to retrieve a serializer for a specific resource, you can do the following:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the use case is, right now, for getting a serializer class or instance. The only thing I can think of is to test that the serializer works as expected, which still requires and adapter...

If you can think of one, I think it would be good to add

Copy link
Member

Choose a reason for hiding this comment

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

Besides that, I'm 💯 LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure. I mainly added this because it was one of the requests of the initial issue. Still might be worth having for the moment.

@bf4
Copy link
Member

bf4 commented Sep 9, 2015

@CodedBeardedSignedTaylor Lemme know if you'd like to pair on bringing this to completion, e.g. rebasing. I'm at http://benjaminfleischer.com/pair

@bf4
Copy link
Member

bf4 commented Sep 9, 2015

TODOs in future PRs:

Serializing a resource section 2

@hiimtaylorjones
Copy link
Contributor Author

Thanks for the offer @bf4! I think I did it correctly this time. Might need to work with you on future stuff though.

@hiimtaylorjones
Copy link
Contributor Author

Also, agreed to the To-Do stuff!

@bf4
Copy link
Member

bf4 commented Sep 9, 2015

@CodedBeardedSignedTaylor are you in the AMS slack? https://amserializers.herokuapp.com/

@hiimtaylorjones
Copy link
Contributor Author

@bf4 Yep! Haven't been on in like a week or so though.

@joaomdmoura
Copy link
Member

Can you squash it into a single commit please? 😁 @CodedBeardedSignedTaylor
Btw nice work here, pretty solid really want to merge this 😄

@hiimtaylorjones
Copy link
Contributor Author

Did that work? I still have merges of the master's repo in my master (thus, they show up as commits). Sorry, I'm kind of bad at squashing commits :(

PericlesTheo and others added 4 commits September 14, 2015 09:17
Fixed indentation in readme under 'using without render'

Get rid of unnecessary instance variables, and implied dependencies.

Fix typo in fieldset exception

Updating wording on cache expiry in README

Extended format for JSONAPI `include` option.

Add lint tests for AR models.
punctuation fixes

as json addition

eliminated redundancy

minor tweaks

README fix

modifications to tutorial

punctuation fixes

as json addition

minor tweaks

modifications to tutorial

punctuation fixes

as json addition

eliminated redundancy

minor tweaks

modifications to tutorial

as json addition

eliminated redundancy

minor tweaks

final edits

final final changes
@NullVoxPopuli
Copy link
Contributor

hey @CodedBeardedSignedTaylor if you could squash this PR, or re-submit a new PR with a single commit, that would be fantastic.

we'll give ya a couple days -- but after that, we'll do it for ya. But we'll be sure to set the commit author to you.

@hiimtaylorjones
Copy link
Contributor Author

#1155 is now the current pull request. Sorry for all the messiness!

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.

6 participants