Add hoc sequences in create_list

[mohammednasser-32]

Fixes #1647

Allow creation of sequence attributes in create_list

This PR is more of a proof of concept, if I get the green light I can add documentation and more tests if needed.

It is working and all tests pass, however I am not confident about the code structure..in this approach FactoryBot.build_sequence is just a syntactical sugar for a Proc, however it will work fine if we pass a Proc too. Is it better to have a dedicated class for this instead?

[mike-burns]

That was fast!

Is it better to have a dedicated class for this instead?

There's a backward compatibility issue to figure out. Imagine this:

class UserWithCallback
  def initialize(name, dob, on_save)
    @name = name
    @dob = dob
    @on_save = on_save
  end

  def save
    user = User.create!(name: @name, dob: @dob)
    @on_save.call(user)
  end
end

FactoryBot.define do
  factory :user_with_callback do
    name { "Zero Cool" }
    dob { "1988-08-10" }
    on_save do
      proc { |user| Logger.info("created user #{user}") }
    end
  end
end

This is, currently, a valid factory_bot definition(*) for a valid Ruby class. Good code? Probably not. Possible? For sure.

So we need to make sure this still works even if we add new functionality.

For that reason, I think we might need a new class.

(* I haven't tried it.)

[mohammednasser-32]

Yes right didn't consider that, thanks for raising it!
here is another approach where I created FactoryBot::AttributeSequence class which encapsulates the proc so we can differentiate between a normal proc and an attribute sequence one

[mohammednasser-32]

Hello @mike-burns! I can see you are no longer maintaining this repository, Can you please tell me who should I follow up regarding this (and the other PR)
(I am really sorry for being obtrusive, I am just keen to contribute here 😅 )

[mike-burns]

You were quick to notice! We're still collecting maintainers, but these people will be in charge: https://github.com/thoughtbot/factory_bot/pull/1651/files May 23, 2024 08:18:44 Mohammed Nasser ***@***.***>:

…

Hello @mike-burns[https://github.com/mike-burns]! I can see you are no longer maintaining this repository, Can you please tell me who should I follow up regarding this (and the other PR[#1639]) (I am really sorry for being obtrusive, I am just keen to contribute here 😅 ) — Reply to this email directly, view it on GitHub[#1650 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAABDRVLAZWMLS5EUSFS72LZDYCFFAVCNFSM6AAAAABHS4SI6CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRXGQYDINRQGE]. You are receiving this because you were mentioned. [Tracking image][https://github.com/notifications/beacon/AAABDRTT6EKSRSGVAD6S3ETZDYCFFA5CNFSM6AAAAABHS4SI6CWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTT6ZWPDS.gif]

[smaboshe]

Hello @mike-burns! I can see you are no longer maintaining this repository, Can you please tell me who should I follow up regarding this (and the other PR) (I am really sorry for being obtrusive, I am just keen to contribute here 😅 )

We're steadily getting up to speed 😅. Thanks in advance for your patience, @mohammednasser-32.

[smaboshe]

Suggested change

	it "create sequencial attribute values" do
	it "creates sequential attribute values" do

[smaboshe]

Suggested change

	context "with sequencial attributes" do
	context "with sequential attributes" do

[smaboshe]

I am curious to know what is the result if expression is nil? What is the default behaviour if a block is not given?

[smaboshe]

What do you think about adding an additional test for the default behaviour. Something like:

context "with an unspecified sequential attribute" do 
    subject { FactoryBot.create_list(:post, 20, title: FactoryBot.build_sequence ) }

    it "creates sequential attribute values" do
      subject.each_with_index do |record, i|
        expect(record.title).to eq ????
      end
    end
  end

Credit: @gkosmo

Open to a discussion about what the default behaviour should ideally be 🙂.

[mohammednasser-32]

Hey @smaboshe , thanks for the review 🙏
yeah I missed this case, I would say we have one of 3 options

• raise an error
• return the index
• return the default value
  They all seem valid so I will apply whatever you believe fits the best 😄

[mohammednasser-32]

@smaboshe @gkosmo looking forward to hear your opinions on this so I can apply it 🙏