Skip to content

Support clearing acronyms from inflector #42475

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

Merged
merged 1 commit into from
Sep 20, 2021

Conversation

odlp
Copy link
Contributor

@odlp odlp commented Jun 14, 2021

Summary

Attempting to clear acronyms in the Inflector breaks the implementation. This results in a TypeError when attempting to set another acronym:

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "activesupport", require: "active_support/all"
end

ActiveSupport::Inflector.inflections do |inflect|
  inflect.clear :acronyms
  inflect.acronym "HTML" # => '[]=': no implicit conversion of String into Integer (TypeError)
end

This is because #clear sets a new Array for @acronyms:

else
instance_variable_set "@#{scope}", []

Rather than a Hash (the default initialized value):

def initialize
@plurals, @singulars, @uncountables, @humans, @acronyms = [], [], Uncountables.new, [], {}

This PR also extends ActiveSupport::Inflector::Inflections#clear / #clear(:all) to clear acronyms, since this wasn't covered.

Other Information

I discovered this issue whilst writing a test for rails/activeresource#361

@ghiculescu
Copy link
Member

I think this does the same thing as #40790

@odlp odlp force-pushed the odlp/inflector-clear-acronyms branch from d9baada to 505a1c6 Compare June 14, 2021 15:34
@odlp
Copy link
Contributor Author

odlp commented Jun 14, 2021

@ghiculescu apologies I didn't spot your PR, you noticed this a while back!

Happy to close this PR / combine efforts / what ever you prefer – it'd be good to get this fixed 😄

@odlp odlp force-pushed the odlp/inflector-clear-acronyms branch from 3afafcf to 452e044 Compare June 14, 2021 15:59
@ghiculescu
Copy link
Member

I don't mind which PR gets merged, as long as the issue is fixed :) It looks like we have ended up on the same implementation 😆 Do you want to just check that the tests I added in mine, also work in yours? If you like, you can add them to this PR and I can close mine.

@odlp
Copy link
Contributor Author

odlp commented Jun 14, 2021

Thanks @ghiculescu, I moved across two tests from your PR.

In particular the extra test which exercises #define_acronym_regex_patterns is called is a great shout 🎉

Copy link
Member

@ghiculescu ghiculescu left a comment

Choose a reason for hiding this comment

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

Looks good to me. Can you please squash?

Previously calling ActiveSupport::Inflector::Inflections.clear(:acronyms)
reset the instance variable to an empty Array, which is not the correct
default. The next time an acronym is added a TypeError is thrown.
@odlp odlp force-pushed the odlp/inflector-clear-acronyms branch from 7d01c62 to a8ae85a Compare June 15, 2021 09:24
@rails-bot
Copy link

rails-bot bot commented Sep 13, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Sep 13, 2021
@rails-bot rails-bot bot closed this Sep 20, 2021
@ghiculescu ghiculescu reopened this Sep 20, 2021
@rails-bot rails-bot bot removed the stale label Sep 20, 2021
@rafaelfranca rafaelfranca merged commit a76344f into rails:main Sep 20, 2021
@rafaelfranca rafaelfranca deleted the odlp/inflector-clear-acronyms branch September 20, 2021 22:04
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.

4 participants