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

fix: ensure the stored field names are always symbols #312

Merged
merged 1 commit into from
Jul 7, 2024

Conversation

flavorjones
Copy link
Collaborator

@flavorjones flavorjones commented Jul 3, 2024

There is code that assumes that @field_names will contain symbols, like this method in ActiveHash::Base:

def respond_to?(method_name, include_private=false)
  super ||
    begin
      config = configuration_for_custom_finder(method_name)
      config && config[:fields].all? do |field|
        field_names.include?(field.to_sym) || field.to_sym == :id
      end
    end
end

This also introduces an explicit test for #field_names.

There is code that assumes that `@field_names` will contain symbols,
like this method in ActiveHash::Base:

    def respond_to?(method_name, include_private=false)
      super ||
        begin
          config = configuration_for_custom_finder(method_name)
          config && config[:fields].all? do |field|
            field_names.include?(field.to_sym) || field.to_sym == :id
          end
        end
    end
Copy link
Collaborator

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Yours makes fewer changes.

I'm a little amused that I asked if we could use strings for field/column names, yet we both doubled down on using symbols for column names.

@kbrock kbrock merged commit cd64cf3 into master Jul 7, 2024
16 checks passed
@flavorjones flavorjones deleted the flavorjones-field-names-should-always-be-symbols branch July 8, 2024 18:16
@kbrock kbrock mentioned this pull request Jul 14, 2024
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.

2 participants