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.

Sign up for GitHub

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

Jump to bottom

Add Action Text compiler #1632

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Add Action Text compiler #1632

merged 1 commit into from
Jan 9, 2024

Conversation

ghiculescu
Copy link
Contributor

Motivation

Adds a compiler for Action Text.

Implementation

I based most of this off the Active Storage compiler. Action Text is not very complex internally so the compiler was not too hard to write.

Tests

Added tests and confirmed they match our home-made RBIs.

@ghiculescu ghiculescu requested a review from a team as a code owner September 1, 2023 00:44
KaanOzkan
KaanOzkan reviewed Sep 1, 2023
View reviewed changes
Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

CI failure and skip on 6.1. Compiler itself looks fine to me.

@KaanOzkan
Copy link
Contributor

I can't replicate the failure locally on 3.0.6 😕

@ghiculescu
Copy link
Contributor Author

Maybe it was just flakey @KaanOzkan

KaanOzkan
KaanOzkan approved these changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

LGTM. Could you add a test for gather_constants also similar to

tapioca/spec/tapioca/dsl/compilers/active_storage_spec.rb

Line 23 in 2f20cd3

it "gathers no constants if there are no ActiveRecord classes" do

@KaanOzkan KaanOzkan added the enhancement New feature or request label Sep 8, 2023
KaanOzkan
KaanOzkan approved these changes Sep 8, 2023
View reviewed changes
Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

Thanks it looks good. Can you please clean up the git history to atomic commits? We don't squash and merge in tapioca and it should help with git blame later on. Sorry for the back and forth didn't notice it earlier.

@ghiculescu
Copy link
Contributor Author

Done @KaanOzkan thank you for all your help!

@ghiculescu
Copy link
Contributor Author

I think the CI errors came from #1604

@KaanOzkan
Copy link
Contributor

CI should be fixed on main now.

@ghiculescu
Copy link
Contributor Author

@KaanOzkan is this ok to merge?

@KaanOzkan KaanOzkan merged commit 1823cd3 into Shopify:main Jan 9, 2024
15 checks passed
@ghiculescu ghiculescu deleted the actiontext branch January 9, 2024 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KaanOzkan KaanOzkan KaanOzkan approved these changes

@st0012 st0012 Awaiting requested review from st0012 st0012 is a code owner automatically assigned from Shopify/sorbet

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ghiculescu @KaanOzkan