-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
RFC: Primary namespace: ActiveModel::Serializer, ActiveModel::Serializers, vs. ActiveModelSerializers #1310
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
- Start Date: (2015-10-29) | ||
- RFC PR: https://github.com/rails-api/active_model_serializers/pull/1310 | ||
- ActiveModelSerializers Issue: https://github.com/rails-api/active_model_serializers/issues/1298 | ||
|
||
# Summary | ||
|
||
Provide a consistent API for the user of the AMS. | ||
|
||
# Motivation | ||
|
||
The actual public API is defined under `ActiveModelSerializers`, | ||
`ActiveModel::Serializer` and `ActiveModel`. | ||
|
||
At the `ActiveModel::Serializer` we have: | ||
|
||
- `ActiveModel::Serializer.config` | ||
- `ActiveModel::Serializer` | ||
|
||
At the `ActiveModelSerializers` we have: | ||
|
||
- `ActiveModelSerializers::Model` | ||
- `ActiveModelSerializers.logger` | ||
|
||
At `ActiveModel` we have: | ||
|
||
- `ActiveModel::SerializableResource` | ||
|
||
The idea here is to provide a single namespace `ActiveModelSerializers` to the user. | ||
Following the same idea we have on other gems like | ||
[Devise](https://github.com/plataformatec/devise/blob/e9c82472ffe7c43a448945f77e034a0e47dde0bb/lib/devise.rb), | ||
[Refile](https://github.com/refile/refile/blob/6b24c293d044862dafbf1bfa4606672a64903aa2/lib/refile.rb) and | ||
[Active Job](https://github.com/rails/rails/blob/30bacc26f8f258b39e12f63fe52389a968d9c1ea/activejob/lib/active_job.rb) | ||
for example. | ||
|
||
This way we are clarifing the boundaries of | ||
[ActiveModelSerializers and Rails](https://github.com/rails-api/active_model_serializers/blob/master/CHANGELOG.md#prehistory) | ||
and make clear that the `ActiveModel::Serializer` class is no longer the primary | ||
behavior of the ActiveModelSerializers. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe note that reasons for naming confusion is in its origin in Rails https://github.com/rails-api/active_model_serializers/blob/master/CHANGELOG.md#prehistory |
||
|
||
# Detailed design | ||
|
||
## New classes and modules organization | ||
|
||
Since this will be a big change we can do this on baby steps, read small pull requests. A | ||
possible approach is: | ||
|
||
- All new code will be in `lib/active_model_serializers/` using | ||
the module namespace `ActiveModelSerializers`. | ||
- Move all content under `ActiveModel::Serializer` to be under | ||
`ActiveModelSerializers`, the adapter is on this steps; | ||
- Move all content under `ActiveModel` to be under `ActiveModelSerializers`, | ||
the `SerializableResource` is on this step; | ||
- Change all public API that doesn't make sense, keeping in mind only to keep | ||
this in the same namespace | ||
- Update the README; | ||
- Update the docs; | ||
|
||
The following table represents the current and the desired classes and modules | ||
at the first moment. | ||
|
||
| Current | Desired | Notes | | ||
|--------------------------------------------------------|--------------------------------------------------|--------------------| | ||
| `ActiveModelSerializers` and `ActiveModel::Serializer` | `ActiveModelSerializers` | The main namespace | | ||
| `ActiveModelSerializers.logger` | `ActiveModelSerializers.logger` || | ||
| `ActiveModelSerializers::Model` | `ActiveModelSerializers::Model` || | ||
| `ActiveModel::SerializableResource` | `ActiveModelSerializers::SerializableResource` || | ||
| `ActiveModel::Serializer` | `ActiveModelSerializers::Serializer` | The name can be discussed in a future pull request. For example, we can rename this to `Resource` [following this idea](https://github.com/rails-api/active_model_serializers/pull/1301/files#r42963185) more info about naming in the next section| | ||
| `ActiveModel::Serializer.config` | `ActiveModelSerializers.config` || | ||
|
||
## Renaming of class and modules | ||
|
||
When moving some content to the new namespace we can find some names that does | ||
not make much sense like `ActiveModelSerializers::Serializer::Adapter::JsonApi`. | ||
Discussion of renaming existing classes / modules and JsonApi objects will | ||
happen in separate pull requests, and issues, and in the google doc | ||
https://docs.google.com/document/d/1rcrJr0sVcazY2Opd_6Kmv1iIwuHbI84s1P_NzFn-05c/edit?usp=sharing | ||
|
||
Some of names already have a definition. | ||
|
||
- Adapters get their own namespace under ActiveModelSerializers. E.g | ||
`ActiveModelSerializers::Adapter` | ||
- Serializers get their own namespace under ActiveModelSerializers. E.g | ||
`ActiveModelSerializers::Serializer` | ||
|
||
## Keeping compatibility | ||
|
||
All moved classes or modules be aliased to their old name and location with | ||
deprecation warnings, such as | ||
[was done for CollectionSerializer](https://github.com/rails-api/active_model_serializers/pull/1251). | ||
|
||
# Drawbacks | ||
|
||
This will be a breaking change, so all users serializers will be broken after a | ||
major bump. | ||
All pull requests will need to rebase since the architeture will change a lot. | ||
|
||
# Alternatives | ||
|
||
We can keep the way it is, and keep in mind to not add another namespace as a | ||
public API. | ||
|
||
# Unresolved questions | ||
|
||
What is the better class name to be used to the class that will be inherited at | ||
the creation of a serializer. This can be discussed in other RFC or directly via | ||
pull request. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
reposting #1298 (comment) here
So, I haven't thought it all the way through, and I just wrote it out without any review, but what I'm thinking right now is something like:
let
AMS
refer toActiveModelSerializers
,AM::S
toActiveModel::Serializer
, andAM::S::A
toActiveModel::Serializer::Adapter
for brevity in the below.ActiveModelSerializers::V08, etc
AMS::Adapter::V08::Json
{JsonApi,Links,Link,Resource,Error, etc}? vs.
JsonApiFormat or even schema validation
re:
ActiveModelSerializers::Current
is just a thought.. perhaps to generate an alternative :)see
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.
@bf4 I think here you go beyond the namespace issue I am right? You mentioned things like
ActiveModelSerializers::V08
andActiveModelSerializers::Current::Store
that I think are new idea of implementations.The Idea of RFC is to define a consistent namespace I will try to focus on it.
At the column desired we have two namespaces as public API
ActiveModelSerializers
andActiveModel
? In your opinion does not make sense a single namespace?Thanks for the feedback
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.
So, that table is also part of #1301 but I think also helps to explain the namespacing
ActiveModel
comes from Rails. Our lib is calledActiveModelSerializers
Rails has anActiveModel::Serializers::JSON
andActiveModel::Serialization
which reflect the history of the two libs, but in any case, I want AMS naming to be less confusing, and its interactions with Rails namespacing makes that harder, in my experience.This is basically identifying the same naming issue as you and coming to the opposite decision. The naming, as it stands, makes things more confusing, in my experience.
So, yes, I'm not changing the scope of the RFC, which is
AMS
vs.AM::S
, but am elaborating on whyAMS
should remain the status quo.Also, I originally wrote that comment as an answer to your question there:
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.
And I guess I wrote some more at https://github.com/rails-api/active_model_serializers/pull/1301/files#r42963185
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.
ActiveModelSerializers
andActiveModel::Serializer
ActiveModel::Serializers
ActiveModelSerializers.logger
ActiveModel::Serializers.logger
ActiveModelSerializers::Model
ActiveModel::Serializers::Model
ActiveModel::SerializableResource
ActiveModel::Serializers::SerializableResource
ActiveModel::Serializer
ActiveModel::Serializers::Serializer
Resource
for example following this ideaActiveModel::Serializer.config
ActiveModel::Serializers.config
This way we will have a single namespace to keep all the AMS into a single namespace
ActiveModel::Serializers
.It seens to me that the actual architeture leads to some confusion to the development team as you can see on this comment this is not clear where to put a new implementation, I feel it too when I am working on the test helpers.
I think if we have a single namespace this will be more clear to the team and to the end users 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.
👍