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

Use build in ExampleBot instead of create + rollback #965

Merged
merged 4 commits into from
Nov 1, 2023

Conversation

jagthedrummer
Copy link
Contributor

@jagthedrummer jagthedrummer commented Sep 5, 2023

Joint PR: bullet-train-co/bullet_train-core#524

This PR adds some dummy ids to some of the *_example factories. (But not any "live" factories that are used outside of ExampleBot.)

Doing this so that the values can be pre-populated for ExampleBot without us needing to actually create live records, clone them, rollback, then reset primary keys. Doing all of that work is slow so it's best to avoid doing it. (And having to reset primary keys for something that we expect to be a normal thing kind of gives me the willies.)

@newstler
Copy link
Contributor

newstler commented Oct 9, 2023

@jagthedrummer Well, I did some research, ran this on a real project, and to be honest I didn't find any noticeable differences besides empty timestamps which I fixed in last commit. Sometimes I fell into SystemStackError (stack level too deep) with complicated includes of relations, but it appears that can be overcome with just setting <relation>_id instead of full relation example. I also changed ids from strings to integers which feels more natural for me to set, although strings were converted to integers anyway.

Not sure if ids should be hardcoded or sequenced like names so when there are list examples the ids would be different?
I like the style of clean hard-coded ids and timestamps, but then probably we should get rid of sequences in names as well. Anyway I think there should be the same style for ids, timestamps and names — either hardcoded or live-generated.

So basically I'm not sure what else can go wrong.

@andrewculver do you remember why we did .create instead of .build with examples generation? I remember we had a huge conversation and quite a strong base under this decision, but all I remember is that you worried that callbacks and such stuff would not fire, but I cannot really find anything that can cause problems in the code.

@jagthedrummer
Copy link
Contributor Author

@newstler, that sounds like good news! I don't have a strong opinion on generating ids/timestamps/etc vs hard coding them. If you prefer hard coding then lets do that. That's probably more stable since there's less that can unexpectedly change. I'll get these PRs rebased and cleaned up.

@jagthedrummer
Copy link
Contributor Author

@newstler I just rebased the branches in both repos and gave everything one last review. Tests are all passing (other than when we test core against the released versions of the gems, and those failures are expected) and I think this is good to go. Do you have any other concerns before we merge this?

@andrewculver
Copy link
Contributor

Let's try it!

Copy link
Contributor

@newstler newstler left a comment

Choose a reason for hiding this comment

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

I also have nothing against it, let's try!

@jagthedrummer jagthedrummer merged commit 4275e17 into main Nov 1, 2023
5 of 8 checks passed
@jagthedrummer jagthedrummer deleted the jeremy/example-bot-build branch November 1, 2023 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants