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

Don't skip over abstract FrozenRecord classes #1099

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amomchilov
Copy link
Contributor

Motivation

On shop-server, we have an abstract! base class that derives from FrozenRecord::Base.

Currently, this class doesn't have an RBI file generated, because the FrozenRecord Tapioca compiler is explicitly skipping over abstract classes.

I don't think there's a reason not to include these, so let's just do it.

Tests

There wasn't a test to check that abstract classes are skipped. If there was one, I would have deleted it. I don't think there's a need to add a test to verify that's we're not not including abstract files, but LMK if you think I should add one anyway :)

@amomchilov amomchilov added the enhancement New feature or request label Aug 5, 2022
@amomchilov amomchilov self-assigned this Aug 5, 2022
@amomchilov amomchilov requested a review from a team as a code owner August 5, 2022 19:56
Copy link
Collaborator

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

LMK if you think I should add one

I think you can add a test under https://github.com/Shopify/tapioca/blob/main/spec/tapioca/dsl/compilers/frozen_record_spec.rb showing that we do not skip abstract classes anymore. At least to show that we do not skip them intentionally.

@rafaelfranca
Copy link
Member

Did we test this PR agains our monolith? I think it might have a reason why we skip abstract classes.

@amomchilov
Copy link
Contributor Author

@rafaelfranca Vinicius suggested that offline. When I have time, I'll run it against core. I'll put this in draft until then.

@amomchilov amomchilov marked this pull request as draft August 9, 2022 16:37
@sambostock
Copy link
Contributor

We also do this for descendants of ActiveRecord::Base. I wonder if it's worth considering that as well?

@amomchilov
Copy link
Contributor Author

amomchilov commented Oct 12, 2022

We actually moved away from having that abstract base class. There were other edge-cases in FrozenRecord that made it more painful than it was worth. I won't have time to investigate this before BFCM, so perhaps we could just close this issue

(Although not skipping them still might make sense as a default, unless we have an explicit reason to want to skip them)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants