Deprecate constructor as a block

[waiting-for-dev]

So far, we have been accepting both a block and the constructor:
keyword argument for providing a constructor. The former will not be
available in 1.0, so we're deprecating it now.

We also prepare the test suite for the change and make naming consistent
using "constructor" where we were using "pre-processor."

[timriley]

Wow, that turned out to be easy! And I much prefer the name "constructor" to "processor" as the way to describe this.

I was just thinking this was all a little familiar, and I realised I had an open PR for this already too, from a while ago: #86. That one looks like it handles the migration away from the default value being a positional arg too.

Would you mind taking a look at that one and seeing if there's anything that's been missed, now that your head is in this space? Maybe we could bring the two together.

[waiting-for-dev]

Hey @timriley, thanks for your review. It seems your PR does more than this one:

• Like this one, it deprecates the constructor as a block.
• It also moves default to a keyword argument.

Would it be ok if we split the whole work into two different PR? I'd merge this one, and then I can prepare another one taking what you did to support the new default: keyword argument. If so, I'll update this one to use Dry::Core::Deprecation#anounce as you did (I wasn't aware of its existence).

[timriley]

@waiting-for-dev Happy for it to be done in two PRs, yep :) Let us know when you're ready for the first one to be reviewed.

@solnic @flash-gordon - what do we feel about releasing these API changes? If they can be handled via deprecations and are therefore not a breaking change, I presume we might be more willing for them to go into a new ordinary 0.x.0 release? With the 1.0 release being the one that removes the deprecation shims?

[waiting-for-dev]

Let us know when you're ready for the first one to be reviewed.

@timriley, it's ready now 🚀

[solnic]

You can just set a logger that would write to log/deprecations.log and this way you could verify if the messages look good. It's an approach we've been using so far.

[waiting-for-dev]

Please, @solnic, take a look at 4e24d68. Do you mean like that? I used StringIO instead of an actual file to not have to mess with the filesystem. If that's what you meant, I'll squash both commits together.

[solnic]

@waiting-for-dev you can move this to spec_helper and use Logger.new("log/deprecations.log") there

[waiting-for-dev]

Unless I'm missing something, this will make the logger persistent across tests, so we'll be sharing a state between them. I think it's not ideal unless we can't avoid it. What do you think creating about creating a helper method so that we can reuse it easily? For instance:

with_logger do |log|
  # expectation
  expect(log).to include('....')
end

[solnic]

@waiting-for-dev OK let's leave it here since it's the only place where we need it for now. Eventually we should have something reusable though, that can be provided by dry-core.

[waiting-for-dev]

OK, it's going to be very soon, as I'm going to use it again in the PR that will deprecate providing the default as positional argument 😄

[waiting-for-dev]

I change the code to deprecate in the same way that #83 is doing. This way we can already remove Compiler#visit_constructor method.

[timriley]

I change the code to deprecate in the same way that #83 is doing. This way we can already remove Compiler#visit_constructor method.

Nice one, I was actually going to ask about this :)

Have you pushed this change up, though? I can't see it in the diff yet.

[waiting-for-dev]

Have you pushed this change up, though? I can't see it in the diff yet.

I just left my terminal while it was prompting for the ssh password 😅 Please, check it out now.

[waiting-for-dev]

Continues at #111 (created branch in dry-rb/dry-configurable repo instead of my fork)