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 InvalidStepDefinition error #79

Merged
merged 3 commits into from
Nov 16, 2017
Merged

Add InvalidStepDefinition error #79

merged 3 commits into from
Nov 16, 2017

Conversation

Morozzzko
Copy link
Member

Resolves #78.

InvalidStepDefinitionError is raised if step definition does not respond
to #call. It covers both missing definitions and improper definitions
(i.e. strings).

Also, OperationResolver used && to chain operations, which lead to
presence of false in the result. Got rid of it. Now it's either
object from the container, or a nil.

Resolves #78.

InvalidStepDefinitionError is raised if step definition does not respond
to `#call`. It covers both missing definitions and improper definitions
(i.e. strings).

Also, OperationResolver used `&&` to chain operations, which lead to
presence of `false` in the result. Got rid of it. Now it's either
object from the container, or a nil.
@Morozzzko Morozzzko changed the title Add InvalidStepDefinitionError Add InvalidStepDefinition error Oct 14, 2017
@GustavoCaso
Copy link
Member

Thank you so much for this. ❤️

What about using MissingStepDefinition instead of InvalidStepDefinition I think bring more value to the actual exception

@Morozzzko
Copy link
Member Author

That is actually what I did, initially. I removed it, however. Thought that just checking for respond_to?(:call) was better than writing some complex logic. Although, now that I think about it, it might have not been that complex. I'm going to try it out and push some updates.

However, there's still a question to answer:

What should be the superclass of the exception? Should it be NameError, since we are looking for a method/proc which does not exist? Should it be ArgumentError since we're passing something that does not exist? Or should it be something generic? Or should it sublcass from InvalidStepDefinition?

I'm thinking InvalidStepDefinition though

@Morozzzko
Copy link
Member Author

Ah, yes, if we wish to stay with both MissingStepDefinition and InvalidStepDefinition (I hope we do), we'll have to do some tricky magic.

The simplest way to tell if the definition is missing is to call #nil?. However, this case will fail if we do it this way.

I'm going to look for a way around this.

@Morozzzko
Copy link
Member Author

Sorry for the delay. Finally managed to find time to finish this.

Apparently, I was silly enough to misunderstand the semantics of assignments inside Ruby class definitions, which held me off for too long.

So, presently, this PR adds two kinds of exceptions:

  • missing step definition (no method & there is nothing in the container)
  • invalid step definition (container contains something that does not respond to #call)

@GustavoCaso
Copy link
Member

@Morozzzko Thank you so much for your work
From my said this looks good. What do you think @timriley?

@timriley
Copy link
Member

timriley commented Nov 7, 2017

Hi folks, I should be able to look at this tomorrow :)

@timriley
Copy link
Member

timriley commented Nov 9, 2017

@Morozzzko This is great stuff, and will definitely help people debug step problems more easily. Thank you!

If you don't mind, I have a couple of minor naming requests: do you think you could rename these to InvalidStepError and MissingStepError? The Error suffix makes them more consistent with Ruby's own error classes. I also don't think we need the word Definition in there, since it's actually the absence or invalidity of the step operation itself that's the problem, not so much that the definition is necessarily error.

Thanks!

@Morozzzko
Copy link
Member Author

Thank you for the review!

The new names are great -- renamed & pushed.

@flash-gordon flash-gordon merged commit a715509 into dry-rb:master Nov 16, 2017
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.

4 participants