null database strategy to disable @javascript handling

[akostadinov]

Summary

Presently there is no nice way to disable @javascript tag handling without writing a custom database strategy class. See #521

This PR adds a null database strategy to allow easily doing so by

Cucumber::Rails::Database.javascript_strategy = :none

I'm planning to add some other enhancements and documentation later to allow custom list of tags but having a null strategy is useful anyway and is a first step.

Types of changes

• Bug fix (non-breaking change which fixes an issue).
• New feature (non-breaking change which adds functionality).
• Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

• I've added tests for my code.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[aurelien-reeves]

What about naming it "none" instead of "null"?

[akostadinov]

@aurelien-reeves , I agree with whatever naming project wants. If the change is in general accepted, I'd go ahead to rename it.

[akostadinov]

I renamed to none.

[aurelien-reeves]

It looks good to me, well done 👌
But as I am not that much into this project, I prefer to let other contributors to review it :)

[akostadinov]

@luke-hill @olleolleolle , could you look at this pull request? It is not big.

Or what are the rules to merge? Can I merge once another member has reviewed and there is some time passed without other comments?

[aurelien-reeves]

@akostadinov could you add an entry in the changelog? And if you had the opportunity to add some documentation regarding those database strategies, that would be a good bonus to that PR :)

@olleolleolle @luke-hill if no further objection, I may merge this soon

[luke-hill]

This feels like it's not a None strategy i.e. one isn't there, but a null strategy. So perhaps we should name it NullStrategy to infer that the responses on the js checkers are nil

[aurelien-reeves]

This feels like it's not a None strategy i.e. one isn't there, but a null strategy. So perhaps we should name it NullStrategy to infer that the responses on the js checkers are nil

Well, it was first named NullStrategy, I ask him to rename it to None weeks ago 😓

I keep feeling this is more "None" than "Null": the strategy is actually nothing, no code. It is like having no strategy at all.

But I may misunderstand it actually 😶

[olleolleolle]

Minor: Perhaps it's not easy to see that the names none and deletion are symbols to be used, or rather, names of strategies. Would they be easier to read if they were quoted?

[akostadinov]

I made a change here, it is clear now, not sure if there would be a nicer way.

[olleolleolle]

Looks good to me.

(I would know what a NullStrategy was, but since the setting to be interacted with is a Ruby Symbol, I'd have to look it up anyway.)

[luke-hill]

@aurelien-reeves so under the hood this sets a few methods. These methods do exist (So they're not none). If these methods didn't exist we'd get an NME.

The OP wanted these methods to deliberately return nil so this is a Blank or Nil Strategy. Because under the hood we change behaviour if people use javascript tags, so we want to amend the transaction methodology

[akostadinov]

So what should name be after all? None, Nil, Null or Blank? I have no personal preference. Actually I chose Null initially because this was the suggestion in one stackoverflow post 😄

[luke-hill]

My vote is for either Nil or Null because those correctly identify that the strategy is setting nil values.

[mattwynne]

The strategy is an instance of the Null Object pattern but we don't have to use that name if we don't want to.

IMO the right name for the symbol is :none as that reads nicely, but we could call the type DoesNothing if we wanted to. They don't have to match.

[akostadinov]

Due to popular demand, I renamed class back to NullStrategy. And :none as strategy selection makes sense to me to indicate that we use none of the javascript testing strategies. :null also makes sense to be consistent with the class name though. Probably none reads a little better. I have arguments in favor of both options so I can change to whatever project owners want.

@luke-hill , added changelog and updated doc to clarify none and deletion are symbols. I have some other modifications ready that can make the feature useful for whatever tags a project decides to use. I think it's better to submit changes in steps and this is the first step. I want to add information to the readme once I have a review and some agreement about the other changes. Is this ok?

Others suggested differing opinions

[aurelien-reeves]

Once again, it looks good to me, once again, thanks a lot @akostadinov

And, once again, I prefer to wait for other review before merging 😅

[akostadinov]

Guys, can we merge in case of no objections?