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

Introduce Scaffolding::Attribute #124

Merged
merged 22 commits into from
Aug 31, 2023
Merged

Conversation

gazayas
Copy link
Contributor

@gazayas gazayas commented Feb 21, 2023

Purpose

The purpose of this PR is to set up bullet_train-super_scaffolding to implement a has_one scaffolder smoothly.
(The issue to create a has_one scaffolderbullet-train-co/bullet_train#76)

Classes to implement

For this to happen, I think it would benefit us to extract some logic from the Transformer and pinpoint where and how we want to build our association lines, the reason being that we Super Scaffold different types of associations to our models based on the attributes passed to bin/super-scaffold. I think where we build the line to eventually place in a model is an important design decision if we want to handle different types of associations dynamically, and I'm hoping to prevent some clutter by adding a couple of new classes.

I propose the following classes:

  1. Scaffolding::Attribute
  2. Scaffolding::AssociationLineBuilder

Scaffolding::Attribute

As you can see with the first commit I've already added this, and there are a lot of diffs in the Transformer by changing name to attribute.name, etc. Although this is technically longer, I was able to cut a lot of the attribute setup at the beginning of the add_attributes_to_various_views method which I believe makes the code much easier to read. I also think the intent is clearer by using this class so whoever reads our code knows that attribute is the key part of this method.

Besides that, we can isolate a lot of the attribute-related logic here. What I'm specifically thinking of is how we declare associations. I was originally thinking of adding a class called Scaffolding::Association or a module named Scaffolding::Association::Helper, but the more I thought about it, Scaffolding::Attribute made the most sense because we don't know if an attribute is technically going to be an association to begin with.

If we can manage association declaration here, I think it will be easier to set up the has_one scaffolder when we get there.

Scaffolding::AssociationLineBuilder

I feel like a lot of this logic will naturally be repeated when we write has_many or belongs_to lines when Super Scaffolding. I haven't quite fleshed this class out yet, but my plan is to work on this next and cut the PR there. Then we could work with these two classes to implement the has_one scaffolder.

@gazayas gazayas changed the title Prepare Transformer for has_one scaffolder Introduce Scaffolding::Attribute Feb 22, 2023
@gazayas
Copy link
Contributor Author

gazayas commented Feb 22, 2023

I decided to mark the PR here as ready for the following reasons

  1. I initially wanted to add both Scaffolding::Attribute and Scaffolding::AssocationLineBuilder in one PR, but I found out there was a decent amount that I was able to extract with just the Attribute class, so I plan on doing a separate PR just for the AssociationLineBuilder.
  2. The conflicts in the transformer aren't actually so bad, but the tests on main are currently failing due to some translation issues, so once that's fixed I plan on merging main.

@gazayas gazayas marked this pull request as ready for review February 22, 2023 12:47
@andrewculver
Copy link
Contributor

@gazayas This looks awesome! Can you resolve the conflicts and then I'll merge it?

@gazayas
Copy link
Contributor Author

gazayas commented Mar 18, 2023

@andrewculver Done!

@jagthedrummer
Copy link
Contributor

@gazayas Looks like new conflicts have cropped up. I'd be happy to merge this once they're resolved.

@jagthedrummer
Copy link
Contributor

@gazayas ping (Sorry to ping you again, I'm just guessing this one fell through the cracks since you've been all over other PRs that I've asked about.)

@gazayas
Copy link
Contributor Author

gazayas commented Aug 10, 2023

Hey @jagthedrummer, I definitely have this one on my radar, it'll just take some careful incision to make sure it works properly so I've been prioritizing the other PRs like CI test failure-related ones. Will ping you when I make some progress!

@jagthedrummer
Copy link
Contributor

OK, that sounds great! Sorry to be a pest.

@gazayas
Copy link
Contributor Author

gazayas commented Aug 10, 2023

You're good 😃

@gazayas
Copy link
Contributor Author

gazayas commented Aug 14, 2023

@jagthedrummer Working on this one, having a hard time though because the super scaffolding tests are passing locally but not on CI for some reason. I know a lot has happened in CI so I hope it's not that, but I'll see what I can do.

@gazayas
Copy link
Contributor Author

gazayas commented Aug 14, 2023

@jagthedrummer Done! That one had my head spinning 😵‍💫

@jagthedrummer
Copy link
Contributor

@gazayas I'm so sorry I didn't merge this immediately last week, but now we have yet another conflict. 🤦

@gazayas
Copy link
Contributor Author

gazayas commented Aug 25, 2023

@jagthedrummer Conflicts resolved! Linking #109 since it was the one that had an affect on the latest merge.

However, the Super Scaffolding partial tests are failing, and it's failing for me locally too on main? Not sure what's going on here, will try to investigate.

@gazayas
Copy link
Contributor Author

gazayas commented Aug 25, 2023

Note to self: The partial tests are working in isolation, but fail when running the entire test setup script.

@gazayas
Copy link
Contributor Author

gazayas commented Aug 25, 2023

I see what's happening, I needed to call this guy in the Attribute class!

スクリーンショット 2023-08-25 20 13 55

THAT was confusing!!

@gazayas
Copy link
Contributor Author

gazayas commented Aug 25, 2023

@jagthedrummer Done!!

Copy link
Contributor

@jagthedrummer jagthedrummer left a comment

Choose a reason for hiding this comment

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

🥳

@jagthedrummer jagthedrummer merged commit 47c11ce into main Aug 31, 2023
1 check passed
@gazayas
Copy link
Contributor Author

gazayas commented Sep 1, 2023

🥳 😭

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.

3 participants