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

Add a warning when doc string is not a string #2922

Merged
merged 3 commits into from Dec 12, 2021

Conversation

pirj
Copy link
Member

@pirj pirj commented Dec 1, 2021

Based on #2921 (comment)

The first argument should be:

For example groups:

  1. a String. Either a textual description (context "in the afternoon"), or a quoted class name (describe "AdminUser")
  2. a Class (describe AdminUser)
  3. a NilClass (context do)

For examples:

  1. a String. A textual description (it 'throws raw eggs at open source maintainers')
  2. a NilClass (specify do)

For shared examples/contexts there is already an Shared example group names can only be a string, symbol or module error.

Am I missing something?

Semi-related side note: we decided not to remove a semi-related described_class in RSpec 4, even though it was previously planned.

@pirj pirj self-assigned this Dec 1, 2021
@pirj pirj added this to the 4.0 milestone Dec 1, 2021
@pirj pirj mentioned this pull request Dec 1, 2021
53 tasks
Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

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

I'm on the fence about symbols, but the rest of this is 👍

it { is_expected.not_to include 4 }
end
group =
with_isolated_stderr do
Copy link
Member

Choose a reason for hiding this comment

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

You're welcome to rewrite/remove these after this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have something like

module = Module.new do
  class << self
    def math?
      true
    end
  end
end

RSpec.describe module do
  it { is_expected.to be_a Module }
  it { is_expected.to be_math }
end

or

RSpec.describe "hello" do
  it { is_expected.to be_a String }
  it { is_expected.not_to include "heaven" }
end

in mind?

Copy link
Member

Choose a reason for hiding this comment

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

A class maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to play with "math class" initially, with "module" it's indeed less funny.

end
end

context "with a Symbol" do
it "returns the symbol" do
expect(value_for :group).to be(:group)
expect(with_isolated_stderr { value_for(:group) }).to be(:group)
Copy link
Member

Choose a reason for hiding this comment

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

Fix rather than isolate please.

expect(group.metadata).not_to include(:symbol)
end

it 'treats the first argument as part of the description when it is a symbol' do
group = RSpec.describe(:symbol)
group = with_isolated_stderr { RSpec.describe(:symbol) }
Copy link
Member

Choose a reason for hiding this comment

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

These should be removed/replaced really?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm on the fence with this, too.
On the one hand, the whole thing is to warn users when they think they add metadata (e.g. RSpec.describe :focus) which is treated as a docstring.
On the other hand, we're just issuing a warning, not hard failing, and we need some specs to prevent regressions in case we decide to rearrange things related to described_class or metadata extraction.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah fair, maybe add an alias for "we expect a warning"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@pirj pirj force-pushed the deprecate-unusual-docstrings branch from 60229f1 to 9ba0ca7 Compare December 10, 2021 23:59
 Undocumented Objects:

(in file: lib/rspec/core/example_group.rb)
RSpec::Core::ExampleGroup.initialize
@pirj pirj force-pushed the deprecate-unusual-docstrings branch from 9474cc0 to 3b07df3 Compare December 11, 2021 00:12
Comment on lines +66 to +68
def with_an_expected_warning
with_isolated_stderr { yield }
end
Copy link
Member

Choose a reason for hiding this comment

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

How do you feel about making this expect an error, e.g. at minimum

Suggested change
def with_an_expected_warning
with_isolated_stderr { yield }
end
def with_an_expected_warning(warning = //)
expect { yield }.to output(warning).to_stderr
end

This would be an actually expected warning then, optionally we could assert on the specifics?

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't work as is, had to rework as

return_value = nil
expect { return_value = yield }.to output(warning).to_stderr
return_value

and it complicates subject_value_for in memoized_helpers_spec.rb, as some usages do output and some don't.
The potential to catch unexpected warnings may not overweight the added complexity.

Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

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

One last suggestion but its non blocking

@pirj pirj merged commit 137a9af into 4-0-dev Dec 12, 2021
@pirj pirj deleted the deprecate-unusual-docstrings branch December 12, 2021 20:08
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.

None yet

2 participants