Skip to content
This repository has been archived by the owner on Nov 30, 2024. It is now read-only.

Emit a special warning when nil is passed to raise_error #1143

Merged
merged 4 commits into from
Nov 27, 2019
Merged

Emit a special warning when nil is passed to raise_error #1143

merged 4 commits into from
Nov 27, 2019

Conversation

pirj
Copy link
Member

@pirj pirj commented Nov 22, 2019

Usage of nil with raise_error is with a high chance unintentional, including negation.

Fixes #1142

@pirj
Add usage examples
feaeb99 
Also slightly fix alignment
@pirj pirj self-assigned this Nov 22, 2019
@pirj pirj requested a review from JonRowe November 22, 2019 18:04
JonRowe
JonRowe reviewed Nov 22, 2019
View reviewed changes
lib/rspec/matchers/built_in/raise_error.rb Outdated Show resolved Hide resolved
lib/rspec/matchers/built_in/raise_error.rb Show resolved Hide resolved
lib/rspec/matchers/built_in/raise_error.rb Outdated Show resolved Hide resolved
lib/rspec/matchers/built_in/raise_error.rb
"This message can be suppressed by setting: " \
"`RSpec::Expectations.configuration.on_potential_false" \
"_positives = :nothing`")
end
Copy link
Member

Choose a reason for hiding this comment

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

Risks or will cause? 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

Risks higher than usual XD

JonRowe
JonRowe reviewed Nov 22, 2019
View reviewed changes
lib/rspec/matchers.rb Outdated Show resolved Hide resolved
@pirj pirj force-pushed the add-special-warning-for-nil-error-passed-to-raise_error branch 2 times, most recently from 656dc0b to 160467a Compare November 22, 2019 20:43
@pirj pirj requested a review from JonRowe November 22, 2019 23:15
@pirj
Add specific warning for nil passed to raise_error
5f10c64 
Usage of `nil` with `raise_error` is with a high chance unintentional,
including negation.

fixes #1142
@pirj pirj force-pushed the add-special-warning-for-nil-error-passed-to-raise_error branch from c0bc5e4 to 5f10c64 Compare November 23, 2019 17:22
@JonRowe JonRowe merged commit 06ca736 into master Nov 27, 2019
@JonRowe JonRowe deleted the add-special-warning-for-nil-error-passed-to-raise_error branch November 27, 2019 15:11
@JonRowe
Copy link
Member

JonRowe commented Nov 27, 2019

Do you want to a change log for this @pirj? Or shall I?

@pirj
Copy link
Member Author

pirj commented Nov 27, 2019

Added in 704f0a26.

JonRowe pushed a commit that referenced this pull request May 8, 2020
@pirj @JonRowe
Emit a special warning when nil is passed to raise_error (#1143)
897877d 
* Add usage examples

* Add specific warning for nil passed to raise_error

Usage of `nil` with `raise_error` is with a high chance unintentional,
including negation.

fixes #1142
@smarlow smarlow mentioned this pull request Oct 30, 2020
Closed
@pirj
Copy link
Member Author

pirj commented Jun 21, 2022

I've received a notification about a note, but the author removed the note. Presumably from @mathieujobin or @mjobin-mdsol.

I wonder if via a config option, we could make this raise an InvalidError instead of a simple warning...

Adding such an option makes total sense to me.

Would you like to send a PR to main to make this configurable?

If you want to go a step further and raise an exception (I'd go with ArgumentError), you can also:

  • send another PR to main, but targeting the release 3.99 that would warn users that the option will be removed, and the exception would become the default
  • send a PR to 4-0-dev with removal of the option and making ArgumentError the default behaviour

@JonRowe Do you support the idea?

@mjobin-mdsol
Copy link

Thanks for the ping @pirj

I can't commit to open a PR with a new config options, I'm sorry. But I created a branch on my end for testing existing projects

mjobin-mdsol@1408453

@JonRowe
Copy link
Member

JonRowe commented Jul 17, 2022

@pirj I'm happy for this idea to be a thing if someone wants to make it so.

@pirj pirj mentioned this pull request Nov 20, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JonRowe JonRowe JonRowe approved these changes

Assignees

@pirj pirj

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unintentional passing of nil to raise_error
3 participants
@pirj @JonRowe @mjobin-mdsol