Skip to content

corrects exposing of inline definitions #503

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Sep 19, 2016
Merged

Conversation

LeFnord
Copy link
Member

@LeFnord LeFnord commented Sep 19, 2016

@@ -288,7 +288,8 @@ def expose_params_from_model(model)
end

def model_name(name)
name.respond_to?(:name) ? name.name.demodulize.camelize : name.split('::').last
name = name.to_s.split('::')[0..-2].join('::').constantize if name.to_s.end_with?('Entity', 'Entities')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we can't just drop all '::', and show model name as is?

Copy link
Member Author

@LeFnord LeFnord Sep 19, 2016

Choose a reason for hiding this comment

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

good point … I did it, cause the name could be very long, see the spec, without it the name it would be: TestDefinitionClass1, it would have all namespaces

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bugagazavr … what is your meaning?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think we can trim symbols if maximum defenition length is reached

Copy link
Contributor

Choose a reason for hiding this comment

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

name.to_s.split('').reverse.take(255).reverse.join

Copy link
Contributor

Choose a reason for hiding this comment

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

also we can add respond_to?(:entity_name) to customize name manually

Copy link
Member Author

Choose a reason for hiding this comment

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

where would it it be set?

Copy link
Contributor

@kzaitsev kzaitsev Sep 19, 2016

Choose a reason for hiding this comment

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

i think this should be placed in model_name method

if name.respond_to?(:entity_name)
  name.entity_name
else
  name.to_s.gsub('::', '').split('').reverse.take(255).reverse.join
end

Copy link
Member Author

@LeFnord LeFnord Sep 19, 2016

Choose a reason for hiding this comment

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

yeap, but where in the entity/API definition would it be set?

Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a class method

length < 42
end.reverse.join
else
name.name.demodulize.camelize
Copy link
Member Author

Choose a reason for hiding this comment

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

I let it, cause without it, it would be a breaking change

- adds changelog entry
- adds option to set definition name
@LeFnord LeFnord merged commit 054c54f into ruby-grape:master Sep 19, 2016
rczjns pushed a commit to rczjns/grape-swagger that referenced this pull request Sep 20, 2016
- adds changelog entry
- adds option to set definition name
@serggl
Copy link
Member

serggl commented Sep 28, 2016

I guess this is a late comment, but why there is 2 different code blocks for parsing model name?

Second question is what is the magic logic of dropping Entity tail in these methods? If I comment this out - specs keep passing...that is confusing

@LeFnord
Copy link
Member Author

LeFnord commented Oct 3, 2016

@serggl … thanks for spotting on it … it seems you have the knowledge to refactore it out,
please can you make a PR, thanks

@serggl
Copy link
Member

serggl commented Oct 3, 2016

@LeFnord sure. On the way

LeFnord added a commit to LeFnord/grape-swagger that referenced this pull request Feb 9, 2019
- adds changelog entry
- adds option to set definition name
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants