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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support uuids in build_stubbed #1583

Merged
merged 1 commit into from
Oct 20, 2023
Merged

Support uuids in build_stubbed #1583

merged 1 commit into from
Oct 20, 2023

Conversation

jaynetics
Copy link

@jaynetics jaynetics commented Jun 24, 2023

This is based on #1514 and fixes #1498.

The only things i did, compared to #1514, are the following:

  • rebased on main
  • added one more guard clause, to ensure that the worst-case result if the relevant rails API is ever removed is the same lack of UUID support that we have currently

The relevant rails API hasn't changed since PR #1514 was created 2 years ago. It would be nice to finally get support for UUIDs. 馃槉

fixes #1498
closes #1514

Co-authored-by: Alexandre Ruban <alexandre@hey.com>
@MatheusRich
Copy link

@mike-burns This looks good. Can you take a look at it?

@mike-burns
Copy link
Contributor

Right, and we're going to need this now that we've outlawed setting the id explicitly.

The thing I've been staring at all afternoon, though, is: this is all very ActiveRecord-specific, which means it should go in factory_bot_rails instead. But I think the original sin was adding this stub strategy to the non-Rails repo in the first place.

OK, I think the plan is: merge this PR, then work on moving the stub strategy into f_b_rails. That will require a lot of finesse and delicacy because I don't want to break any existing code in the process.

Thanks for listening as I talk this through.

Hey @jaynetics @alexandreruban and @StefSchenkelaars , thank you all for the work you put into this!

@mike-burns
Copy link
Contributor

(Current status: figuring out why CI isn't running. sigh)

@MatheusRich
Copy link

@mike-burns might be related #1593

@mike-burns
Copy link
Contributor

Thanks for being on top of things, @MatheusRich! But if that's the case, why aren't those CI checks running?

@mike-burns
Copy link
Contributor

I pushed this branch to the main repo and watched CI pass: https://github.com/thoughtbot/factory_bot/actions/runs/6592716225

Going to merge bypassing CI for this PR.

@mike-burns mike-burns merged commit 9b9b24f into thoughtbot:main Oct 20, 2023
@jaynetics jaynetics deleted the build-stub-with-uuids branch October 22, 2023 14:56
@jaynetics
Copy link
Author

@mike-burns thanks for merging!

this is all very ActiveRecord-specific, which means it should go in factory_bot_rails instead. But I think the original sin was adding this stub strategy to the non-Rails repo in the first place.
OK, I think the plan is: merge this PR, then work on moving the stub strategy into f_b_rails.

if you want to support build_stubbed for ORMs other than activerecord in the future, a consistent solution might be easier to achieve if its all in the same base repo.

either way, f_b_rails doesn't seem quite the right place for it because some people use activerecord without rails (e.g. on sinatra), and some people use rails without activerecord (e.g. using sequel instead). currently, f_b_rails crashes if rails is not present (despite having only activerecord, not rails, in its gemspec dependencies), so moving features to there might affect a few people.

@mike-burns
Copy link
Contributor

Yeah fair - perhaps the actual original problem was splitting out a Rails-specific gem to begin with, with all the vagueness that "Rails" implies.

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.

build_stubbed does not fill id for uuid column type
4 participants