Skip to content

Deprecate the custom exist matcher#68

Merged
adamstegman merged 3 commits intoalexrothenberg:mainfrom
adamstegman:rspec-exist
Nov 29, 2023
Merged

Deprecate the custom exist matcher#68
adamstegman merged 3 commits intoalexrothenberg:mainfrom
adamstegman:rspec-exist

Conversation

@adamstegman
Copy link
Collaborator

Prefer the one built-in by RSpec instead. Fall back to the built-in matcher for default behavior when given a filename.

Fixes #60

Prefer the one built-in by RSpec instead.
Fall back to the built-in matcher for default behavior when given a filename.
Copy link
Owner

@alexrothenberg alexrothenberg left a comment

Choose a reason for hiding this comment

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

:shipit:

@adamstegman I added you as an collaborator here + owner in rubygems in case you want to merge and release this yourself

@adamstegman adamstegman merged commit 19fd0cc into alexrothenberg:main Nov 29, 2023
@adamstegman adamstegman deleted the rspec-exist branch November 29, 2023 22:02
@pirj
Copy link
Contributor

pirj commented Dec 4, 2023

This loud deprecation broke RSpec's own CI, which is kept warning-free.

It turned out funny that rspec-rails's specs were relying on the Ammeter's implementation of the exist matcher.

     Failure/Error: expect(generator_spec).to exist
       expected "/home/pirj/source/rspec-rails/tmp/spec/generator/posts_generator_spec.rb" to exist but it does not respond to either `exist?` or `exists?`

Most certainly, we just need to clean this up in rspec-rails specs and use Pathname as you suggest.

Is there a canonical way to transform a bunch of existing specs to work without warnings?

      subject { file('spec/models/posts_spec.rb') }

      it { is_expected.to contain /require 'spec_helper'/ }
      it { is_expected.to exist }

What if we change the file helper method to return a Pathname instead of just a string?

        def file relative
-          File.expand_path(relative, destination_root)
+          Pathname.new(File.expand_path(relative, destination_root))

This worked to fix rspec-rails specs.
Just one of Ammeter's own specs fail. Are there any side effects, or is it fine to adjust specs? I can send a PR.

  1) Ammeter::RSpec::Rails::GeneratorExampleGroup an instance of the group working with files should use destination to find relative root file
     Failure/Error: expect(group.file('app/model/post.rb')).to eq "#{path_to_gem_root_tmp}/app/model/post.rb"

       expected: "/home/pirj/source/tmp/app/model/post.rb"
            got: #<Pathname:/home/pirj/source/tmp/app/model/post.rb>

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -"/home/pirj/source/tmp/app/model/post.rb"
       +#<Pathname:/home/pirj/source/tmp/app/model/post.rb>

to fix it:

- expect(group.file('app/model/post.rb')).to eq "#{path_to_gem_root_tmp}/app/model/post.rb"
+ expect(group.file('app/model/post.rb').to_path).to eq "#{path_to_gem_root_tmp}/app/model/post.rb"

@pirj
Copy link
Contributor

pirj commented Dec 4, 2023

#70

@adamstegman
Copy link
Collaborator Author

Great idea! It feels pretty safe to me in general, but worth calling out as a potential breaking change.

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.

ammeter breaks RSpec's built-in exist matcher

3 participants