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

Move ApiObjects to JsonApi namespace/folder #1529

Closed
wants to merge 1 commit into from

Conversation

groyoh
Copy link
Member

@groyoh groyoh commented Feb 20, 2016

Follow up to
https://github.com/rails-api/active_model_serializers/pull/1504/files#r52796473.
Moved the Relationship and ResourceIdentifier to the JsonApi
namespace/folder and slightly refactored the Relationship code.

@groyoh groyoh force-pushed the refactor_api_objects branch from 8f6d5aa to ccca84b Compare February 20, 2016 12:19
class Serializer
module Adapter
class JsonApi
class Relationship
Copy link
Member Author

Choose a reason for hiding this comment

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

@bf4 following your comment I was not sure if they should still be namespace by ApiObjects or not.

Copy link
Member

Choose a reason for hiding this comment

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

namespacing it under ApiObjects seems unnecessary to me.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Under JsonApi is fine, see my errors pr

On Mon, Feb 22, 2016 at 9:56 PM Ben Mills [email protected] wrote:

In lib/active_model/serializer/adapter/json_api/relationship.rb
#1529 (comment)
:

@@ -0,0 +1,53 @@
+module ActiveModel

  • class Serializer
  • module Adapter
  •  class JsonApi
    
  •    class Relationship
    

namespacing it under ApiObjects seems unnecessary to me.


Reply to this email directly or view it on GitHub
https://github.com/rails-api/active_model_serializers/pull/1529/files#r53733792
.

@groyoh
Copy link
Member Author

groyoh commented Feb 20, 2016

The tests files were not changed/moved because they would clash with some other test name (e.g. test/adapter/json_api/relationship_test.rb) so I kept them under test/adapter/json_api/api_objects folder.

@remear
Copy link
Member

remear commented Feb 23, 2016

What if the tests from test/adapter/json_api/api_objects were merged into the files of the same name in the parent directory?

@groyoh
Copy link
Member Author

groyoh commented Feb 23, 2016

And have two different group of tests in the same test file? I don't think that's appropriate. To be clear here test/adapter/json_api/relationship_test.rb tests the jsonapi adapter behavior when using relationship and test/adapter/json_api/api_objects/relationship_test.rb tests the relationship object itself.
Maybe there is a better solution here.

@remear
Copy link
Member

remear commented Feb 23, 2016

Do you have a preference on how you'd like it to be structured?

@groyoh
Copy link
Member Author

groyoh commented Feb 26, 2016

@remear not really. Any suggestion would be good to me. I would just not like to merge the two different concerns into the same file.

Follow up to
https://github.com/rails-api/active_model_serializers/pull/1504/files#r52796473.
Moved the Relationship and ResourceIdentifier to the JsonApi
namespace/folder and slightly refactored the Relationship code.
@groyoh groyoh force-pushed the refactor_api_objects branch from ccca84b to e076111 Compare February 28, 2016 12:16
@groyoh
Copy link
Member Author

groyoh commented Feb 28, 2016

@bf4 rebased on master and moved it to ActiveModelSerializers namespace.

@@ -1,87 +1,84 @@
require 'test_helper'
Copy link
Member

Choose a reason for hiding this comment

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

Let's move these downs down a level and delete the api_objects directory

@groyoh groyoh mentioned this pull request Feb 29, 2016
@groyoh
Copy link
Member Author

groyoh commented Mar 3, 2016

Closing in favor of #1543

@groyoh groyoh closed this Mar 3, 2016
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