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

Merged
merged 10 commits into from
Aug 21, 2024
Merged

Conversation

hatsu38
Copy link
Contributor

@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
Contributor 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
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.

thanks

@kbrock
Copy link
Collaborator

kbrock commented Jul 14, 2024

@flavorjones Mike, Symbol.name was introduced in ruby 3.0. Do you have an opinion on how to resolve?

I almost think dropping Ruby <3.0 (and therefore rails rails <=6.0. It sounds drastic but they are all EOL since early 2023, and this will work for them, just not the new column_names.

OTOH/ we could use Symbol.to_s and not worry about the frozen world.

@flavorjones
Copy link
Collaborator

Oof, hmm. Well for the record: Rails 6.1 is still supported, and it still technically supports Ruby 2.7. Totally annoying.

But: you're right that this is a new feature, and Rails 6.1 is only supported for security updates at this point. So I agree it is feasible and probably preferable to drop support for Ruby < 3.0 in the next minor release.

@kbrock
Copy link
Collaborator

kbrock commented Jul 17, 2024

w

Comment on lines 124 to 126
it "returns an array of column names" do
expect(Country.column_names).to eq(["name", "iso_name", "size"])
end
Copy link
Collaborator

@kbrock kbrock Jul 19, 2024

Choose a reason for hiding this comment

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

@flavorjones do we want to just conditionally do this?

Suggested change
it "returns an array of column names" do
expect(Country.column_names).to eq(["name", "iso_name", "size"])
end
it "returns an array of column names" do
skip unless RUBY_VERSION >= "3.0"
expect(Country.column_names).to eq(["name", "iso_name", "size"])
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems fine to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I commited! 9679a89

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.

Hard fought. This looks good.

@flavorjones The githubs humbly requests you take a look at this too.

@kbrock kbrock merged commit 627dc17 into active-hash:master Aug 21, 2024
16 checks passed
@hatsu38 hatsu38 deleted the add-column_names-method branch August 22, 2024 02:26
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