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.

Sign up for GitHub

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

Jump to bottom

method missing to_ary #2370

Merged
merged 9 commits into from
Nov 20, 2023
Merged

method missing to_ary #2370

merged 9 commits into from
Nov 20, 2023

Conversation

ericproulx
Copy link
Contributor

While profiling #2363, I found that to_ary is leaking as Allocated String Report.
It has been fixed in by #466 but lost since.

@ericproulx ericproulx changed the title Add to_ary nil method missing to_ary Nov 14, 2023
@ericproulx ericproulx requested a review from dblock November 14, 2023 14:28
dblock
dblock requested changes Nov 15, 2023
View reviewed changes
Copy link
Member

@dblock dblock left a comment •
edited
Loading

Choose a reason for hiding this comment

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

When does this happen? Add a test or we'll break it again!

@ericproulx
Copy link
Contributor Author

When does this happen? Add a test or we'll break it again!

Its happening on implicit conversions. Anyway, the method_missing was introduced in 2016 to deprecate route_xyz methods. I would consider remove the entire missing and warning part. That way, we won't have to deal with the to_ary

@dblock
Copy link
Member

dblock commented Nov 19, 2023

Should we add a section into UPGRADING and save someone who wasn't paying attention to the deprecation some trouble?

dblock
dblock approved these changes Nov 20, 2023
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

One more period and let's merge. Thanks for doing this.

README.md Outdated
@@ -3506,7 +3506,7 @@ TwitterAPI::routes[0].description # => 'Includes custom settings.'
TwitterAPI::routes[0].settings[:custom] # => { key: 'value' }
```

Note that `Route#route_xyz` methods have been deprecated since 0.15.0.
Note that `Route#route_xyz` methods have been deprecated since 0.15.0 and removed since 2.0.1
Copy link
Member

Choose a reason for hiding this comment

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

Add missing period.

@dblock dblock merged commit 3f01d03 into ruby-grape:master Nov 20, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ericproulx @dblock