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 support for custom failure messages in Spec expectations #10127

Merged
merged 3 commits into from
Jan 19, 2021

Conversation

Fryguy
Copy link
Contributor

@Fryguy Fryguy commented Dec 21, 2020

This PR introduces a breaking change if people happened to be using file and line as positional parameters, which is why I'd like to get this in before 1.0. The reason I introduced this breaking change is because as a user I'd expect the failure message to come immediately after the expectation (as it is in RSpec), and I believe it will be much more likely for users to use custom error messages than file and line (based on my experience in Ruby with RSpec).

The fix for the file and line change is very straightforward. In this PR I've fixed the core specs by using keyword notation, since core had both keyword-style and positional-style, and keyword-style seemed cleaner. I had even considered "forcing" keywords to be used for the file and line parameters, but thought that might warrant some discussion first.

See also the brief discussion in this thread: https://forum.crystal-lang.org/t/spec-assertion-with-custom-message/1781

@straight-shoota
Copy link
Member

I would make file and line named arguments, even without this change. With it, even more so.

@Fryguy
Copy link
Contributor Author

Fryguy commented Dec 21, 2020

@straight-shoota I agree (just to clarify, when you say named arguments, you mean the "forcing" of the caller to use keywords, right?)

@oprypin
Copy link
Member

oprypin commented Dec 21, 2020

Probably should split the file,line named arguments thing into a separate PR, not to taint this one as breaking-change.

@straight-shoota
Copy link
Member

@oprypin failure_message replaces file as second positional argument, so it's definitely a breaking change. And it can even break silently. Although that shouldn't be too much of a problem because typically both file and line are passed and then the argument types don't match. And it's also "just" spec output, so if it fails and prints a message instead of a file name, that's not a big deal. You can easily fix it.

@bcardiff bcardiff added this to the 0.36.0 milestone Jan 18, 2021
@bcardiff
Copy link
Member

Can we add the following declarations to get deprecation warnings instead of compile-time errors?

    @[Deprecated("Use named arguments .should expectation, file: file, line: line")]
    def should(expectation, file, line)
      should(expectation, nil, file: file, line: line)
    end

    @[Deprecated("Use named arguments .should_not expectation, file: file, line: line")]
    def should_not(expectation, file, line)
      should_not(expectation, nil, file: file, line: line)
    end

There are some shards that are using file, line for custom error messages and I would rather offer a transition here.

@bcardiff bcardiff merged commit 134a117 into crystal-lang:master Jan 19, 2021
@Fryguy Fryguy deleted the spec_message branch January 19, 2021 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants