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

Add column names method #311

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

hatsu38
Copy link

@hatsu38 hatsu38 commented Jul 3, 2024

This pull request adds the column_names method to the ActiveHash gem.

The column_names method is analogous to the one found in Rails' ActiveRecord, allowing users to retrieve the names of columns defined in an ActiveHash model.

Added column_names method to ActiveHash::Base class.
The method returns an array of string column names, similar to ActiveRecord's column_names method.

Country.column_names
# => ["id", "name", "code"]

close #310

Copy link
Collaborator

@flavorjones flavorjones left a comment

Choose a reason for hiding this comment

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

@hatsu38 Thank you for submitting this!

If the goal here is to make sure active hash integrates with the CSV gem, can you please add a new spec that verifies this is working as expected? I'd suggest creating a new directory, spec/integration, and a new spec file.

I would also be interested in a short "how to generate CSV" in the README.md file.

lib/active_hash/base.rb Show resolved Hide resolved
lib/active_hash/base.rb Outdated Show resolved Hide resolved
@flavorjones
Copy link
Collaborator

I would also like to see a short explicit test in spec/active_hash/base_spec.rb like the one I added in #312 for #field_names. Thank you!

@kbrock
Copy link
Collaborator

kbrock commented Jul 3, 2024

Rails does not have field_names, instead they have column_names. So there is a difference.
Rails explicitly states that column_names is an array of strings.

I'd feel more comfortable with just doing the work for column_names.
@flavorjones what is your take?

def column_names
  field_names.map(&:name)
end

Also looks like field_names << field is not ensuring that a symbol goes in there, but comparison routines assume it is a symbol. I'll create a PR for that.

Using Symbol#name instead of Symbol#to_s avoids creating a new string
for each invocation, improving performance and memory efficiency.
@flavorjones
Copy link
Collaborator

@kbrock I agree, let's not cache column names, and always iterate over field names when it's called. @hatsu38 Can you make that change?

Ruby versions prior to 3.0 do not have the name method in the Symbol
class.
@hatsu38 hatsu38 requested a review from flavorjones July 8, 2024 00:08
Comment on lines 67 to 69
field_names.map do |field|
field.respond_to?(:name) ? field.name : field.to_s.freeze
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now field names are all symbols, so I think we can simplify this:

Suggested change
field_names.map do |field|
field.respond_to?(:name) ? field.name : field.to_s.freeze
end
fields.map(&:name)

Does that work for you?

Copy link
Author

Choose a reason for hiding this comment

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

After merging the latest master, the code worked perfectly! 0306858

@hatsu38 hatsu38 requested a review from kbrock July 8, 2024 07:50
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.

Add column_names Method to ActiveHash
3 participants