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

Id equal to model name when root is used in Entity #251

Closed
wants to merge 4 commits into from

Conversation

aitortomas
Copy link

See issue #250

@aitortomas aitortomas changed the title Fixed issue with id being different than model name when root is used Id equal to model name when root is used in Entity Apr 23, 2015
@aitortomas
Copy link
Author

With my fix, the schema in Swagger-UI would show with the class name from the entity.
Should it show the defined "root" if existing?
I am open to discussion.

@dblock
Copy link
Member

dblock commented Apr 23, 2015

So this was a misunderstanding of the spec?

Fix the build please (rubocop), update changelog.

@aitortomas
Copy link
Author

Yes, it looks like a misunderstanding of the specs, at least for the case in which root is defined in the entities.

@dblock
Copy link
Member

dblock commented Apr 27, 2015

Merged via tim-vandecasteele@834ee01.

@dblock dblock closed this Apr 27, 2015
@iangreenleaf
Copy link
Contributor

This now breaks my entity references because root gets used in the models object but not in apis[].operations[].type.

Should @root be used at all by default? Seems like it's internal data and meant for defining the formatting, not for defining the model ID.

@aitortomas
Copy link
Author

I see what you mean, I can put the root logic inside the parse_entity_name method and it will work.
Despite that, I am open to discussion regarding if root should actually be used or not.

@dblock
Copy link
Member

dblock commented Apr 29, 2015

@iangreenleaf and @aitortomas Can you please help resolve this? Lets make sure we don't break backward compatibility or have a way out documented in UPGRADING?

@aitortomas
Copy link
Author

@iangreenleaf
Copy link
Contributor

Looks like #254 is a good fix if we're keeping the root logic. I'm still on the side of removing it, though. @dblock @aitortomas do either of you know why it was added in the first place? I found the commit that introduced it (2747bb2) but I don't know if there's an associated issue and discussion.

It seems like the kind of behavior that might be convenient for some apps, but not everyone in general. The fact that I want my output wrapped as {xyz: {…}} doesn't necessarily mean I want xyz to be my canonical model name.

Removing this would break backwards compatibility with anyone depending on it, but I think we could work out a simple upgrade path. For example, people who wanted this behavior could redefine entity_name to something like this:

def entity_name
  super || @root
end

@dblock
Copy link
Member

dblock commented Apr 30, 2015

To answer your question, honestly I don't know the background for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants