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

Check RBI syntax in DslCompiler test helper #1870

Merged
merged 1 commit into from Apr 30, 2024

Conversation

bdewater
Copy link
Contributor

@bdewater bdewater commented Apr 15, 2024

Motivation

Prevent #1867 from happening again.

Closes #554 (supersedes it)

Implementation

Sorbet sigs are valid Ruby syntax, so the bad code that aforementioned PR fixes will fail:

% bin/test spec/tapioca/dsl/compilers/active_record_relations_spec.rb
Started with run options --seed 10501

Tapioca
  Dsl
    Compilers
      ActiveRecordRelations
        initialize
          it gathers only ActiveRecord constants with no abstract classes PASS (0.39s)
          it gathers no constants if there are no ActiveRecord classes    PASS (0.38s)

Tapioca
  Dsl
    Compilers
      ActiveRecordRelations
        decorate
          it generates handles composite primary keys                    ERROR (0.76s)
Minitest::UnexpectedError:         SyntaxError: Expected generated RBI file for `Post` to not have any parsing errors.
        
        Got these parsing errors:
        
        -e:55: unexpected token ")" https://srb.help/2001
            55 |    sig { params(args: NilClass, block: T.proc.params(object: ::Post).void)).returns(T.nilable(::Post)) }
                                                                                           ^
        Errors: 1
        
        
            spec/tapioca/dsl/compilers/active_record_relations_spec.rb:1434:in `block (3 levels) in <class:ActiveRecordRelationsSpec>'

          it generates proper relation classes and modules               ERROR (0.57s)
Minitest::UnexpectedError:         SyntaxError: Expected generated RBI file for `Post` to not have any parsing errors.
        
        Got these parsing errors:
        
        -e:55: unexpected token ")" https://srb.help/2001
            55 |    sig { params(args: NilClass, block: T.proc.params(object: ::Post).void)).returns(T.nilable(::Post)) }
                                                                                           ^
        Errors: 1
        
        
            spec/tapioca/dsl/compilers/active_record_relations_spec.rb:738:in `block (3 levels) in <class:ActiveRecordRelationsSpec>'

Tests

Added tests.

@bdewater bdewater requested a review from a team as a code owner April 15, 2024 15:42
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.

I was thinking of running type checking instead if it's not too slow to cover other potential issues.

@Morriar
Copy link
Collaborator

Morriar commented Apr 15, 2024

I agree with @KaanOzkan, checking the generated files with Sorbet will provide more value.

You can take a look at this old draft trying to do exactly that: #554.

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.

See my previous comment.

KaanOzkan
KaanOzkan previously approved these changes Apr 22, 2024
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

@KaanOzkan KaanOzkan dismissed their stale review April 22, 2024 14:04

Extra questions

Co-authored-by: Ufuk Kayserilioglu <ufuk.kayserilioglu@shopify.com>
@KaanOzkan KaanOzkan added the enhancement New feature or request label Apr 22, 2024
@KaanOzkan KaanOzkan merged commit a428e28 into Shopify:main Apr 30, 2024
18 of 19 checks passed
@bdewater bdewater deleted the check-rbi-syntax branch May 3, 2024 14:48
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.

None yet

3 participants