Make `Config#finalize!` finalize itself recurively and allow freezing values #105

ojab · 2021-04-10T15:01:24Z

Fixes #104

solnic

Thanks for tackling this! I left one comment. In general, we're trying to wrap up dry-configurable 1.0.0 and since it's used in a bunch of projects (+ we want to use it in hanami 2.0) we need to 100000% sure we're not breaking anything. Some behaviors are really tricky so we probably have to check your branch against dry-schema and dry-system too before merging.

spec/integration/dry/configurable/setting_spec.rb

flash-gordon · 2021-04-12T08:48:02Z

lib/dry/configurable/setting.rb

@@ -94,6 +96,12 @@ def pristine
        with(input: Undefined)
      end

+      # @api private
+      def finalize!
+        value.is_a?(Config) ? value.finalize! : value.freeze


I don't think that freezing values is the right way. You can't know in advance what values are, it can be anything. For instance, if it's a module or a class (a reasonable assumption for config!) then you won't be able to define new constants there. I don't this is expected.

I thought that it would be questionable approach :)

My reasoning is that if we're finalizing config — it owns all the values and we should take that into account. IMHO

irb(main):005:0> App.config.database.dsn => "sqlite:memory" irb(main):006:0> App.config.database.dsn << 'qwe' => "sqlite:memoryqwe"

is quite unexpected after we #finalize!, especially since documentation says # Note that finalized configs cannot be changed. & changelog says Introduce 'Configurable.finalize!' which freezes config and its dependencies.

Yeah, I understand the reasoning, I would be happy to have everything immutable in the first place. But my position on this it's not up to libraries to mutate objects provided by the user. Freezing is a type of mutation so this rule applies. There are two problems: 1) it's unexpected 2) you can't opt-out of this behavior. I don't think we should be unnecessarily smart about this. We could have a warning shown when a non-frozen value passed to the config but then I guess there'd be a ton of them in a typical project, I'm not sure if this would be helpful.

Right now semantics of #finalize! is not clearly defined, so what is the expected behavior? Freeze just config structure, so we're protected from deleting of used keys?
From the changelog entry I expected that dependencies are config values, not sure what was meant by dependencies if they're not.
Implicit mutation is certainly not a good thing, but #finalize! is explicit user's decision and if semantic would be defined as freeze all the keys & values — why not? If you want to opt-out — just don't call #finalize!.

If we still need to freeze the structure without values — can we change #freeze to freeze the config without values recursively, so the structure is set in stone, and make #finalize! to freeze also the values?

Right now #finalize! seems broken anyway, because freezing only top-level keys has doubtful usefullness.

It's not that simple. finalized! is called by dry-system and it's an essinatial part in the lifecycle, you can't skip it. When people install new version of dry-system witth current change they'll get new semantics for the "why not?" reason. I agree it shouldn't break things but the problem is we can't be sure and our semantics will be different from what's understood as a default in ruby: calling .freeze on arrays and hashes doesn't freeze values.

WRT recursive finalize! and freezing config structure I don't see problems with this. I agree it's not expected config structure can be changed after freezing.

Re dry-system, I believe it no longer depends on this hook (in master) but yeah, if the behavior changes here it'll break current dry-system releases.

Yeah, I checked, it's not called. It's not clear whether it should be though. All in all, I think we could have freezing values as an option .finalize!(freeze_values: true). We can debate if it should be true or false by default but either way, it provides an opt-in/opt-out mechanism which is good. WDYT?

You're right that dry-system doesn't call finalize! on its config at the moment. This would be worth investigating before we ship 1.0. I'll file an issue.

In fact, I'm not sure if we really finalize configs much at all across the dry-rb gems at the moment.

Having a freeze_values option to opt in or out of freezing non-Config values like a sensible way to go here 👍🏼

Issue for dry-system here: dry-rb/dry-system#168

ojab · 2021-04-12T12:09:40Z

Should I just check dry-schema/dry-system locally or make draft PRs to them using this branch?

solnic · 2021-04-13T05:22:07Z

Should I just check dry-schema/dry-system locally or make draft PRs to them using this branch?

Running locally should be enough. CI will later on run specs against dry-configuration master to double-check.

timriley · 2021-04-13T23:00:24Z

lib/dry/configurable/setting.rb

-        evaluate if input_defined?
+        value if input_defined?


I can see why you've removed the private #evaluate method here, to keep all the logic in #value itself, but the net result is that this line here reads a lot worse – it's just a random noun hanging out there on its own line, rather than a verb describing the outcome of the method (which, in this case, is actually about a side effect, so a verb makes sense).

Is this just a change you just made along the way because you thought it would be nice, or is it critical to your work in this PR? I think I'd prefer to see it left unchanged and have this PR focus on the freezing behavior only.

@value ||= instead of evaluated? leads to evaluating value during cloneable? check from initialize_copy which fails if finalize!(freeze_values: true) was called. I don't quite like how it looks either, added alias evaluate -> value.

ojab · 2021-04-17T14:46:10Z

Sorry for delay, changed it to finalize!(freeze_values: false), checked dry-system & dry-schema from git, specs are passing.

ojab · 2021-04-17T14:50:23Z

lib/dry/configurable/instance_methods.rb

@@ -33,9 +33,7 @@ module InstanceMethods
      # Finalize the config and freeze the object
      #
      # @api public
-      def finalize!
-        return self if frozen?


#frozen? checks would be unreliable now and I don't think that it's performance-critical part, just drop them.

@ojab I wonder if this should just be idempotent or maybe if we should raise an error if a config is finalized and something tries to finalize it again 🤔

Oh my, missed this comment. We can raise an error, but now it's idempotent, so didn't changed it.
IMHO raising an error would only lead to people figuring out how to workaround it and will not catch any real issues.

ojab · 2021-06-09T14:45:19Z

Rebased, anything to be done to get it merged?

spec/integration/dry/configurable/setting_spec.rb

ojab · 2021-07-14T15:52:38Z

Gentle ping.

solnic

Would it be possible to just use freeze method and remove finalize!? Then Config#freeze could do its thing and behave just like Hash#freeze and THEN we could have Config#deep_freeze to make things simpler and more explicit.

This would be more flexible and possibly future-proof given that there's a discussion about adding Object#deep_freeze to Ruby.

I think getting this functionality done right is going to be crucial, that's why I'd prefer to really think it through. I should mention I'm integrating dry-configurable into rom-rb's core library and it's going to play major role there. So now all of a sudden I'm deeply interested in having proper config/settings/values freezing 😅 I used finalize! there and of course it didn't do what I assumed it would.

ojab · 2021-07-20T14:52:15Z

Then Config#freeze could do its thing and behave just like Hash#freeze

umm, do you mean with recursion or without? Since it's not recursive/behaves like Hash#freeze right now and IMHO it's pretty useless, making it recursive would make it different from built-in objects. AFAIU it's finalize! name for that exact reason.
OTOH there is no discussion in the PR and it was 4 years ago + it's useless right now, so making it recursive would be sound.

and THEN we could have Config#deep_freeze to make things simpler and more explicit
This would be more flexible and possibly future-proof given that there's a discussion about adding Object#deep_freeze to Ruby.

Heh, maybe with ractors #deep_freeze proposal would be more successful. But #deep_freeze is not in ruby yet, so there is non-zero chance to have different semantics and/or breaking change in dry-configurable when it will be.
Also #deep_freeze implies that we're freezing everything recursively, which wouldn't be the case for the nested objects in setting values (e. g. for Config.new(x: ['string']).deep_freeze only array would be frozen and 'string' wouldn't be), IMHO this would be unexpected.

And underlying implementation probably wouldn't change (two separate recursive freezers would be more complicated, I guess), so feel free to merge it as is and make finalize! private + bikeshed public names later 😜

AMHOL · 2021-07-21T01:17:47Z

#finalize! was just a pattern that got copied over from rom-rb into dry-container and then here in dry-configurable, wasn't much thought that went into it other than that. I just thought of #finalize! as a method to be called at the end of the setup-phaze of an application, before any requests are made, and it made sense as a good place to freeze values.

ojab · 2021-07-21T19:06:51Z

Oh, I'm not really accustomed to dry/rom ecosystem, with prior art it's perfectly logical naming without further justification.

made sense as a good place to freeze values.

part make me think that maybe we should break API and make it freeze values by default, since we're not 1.0 yet ._. (dunno if dry ecosystem follows semver (google says dry-rb/dry-rb.org#112 (comment), so API could be broken easily /shrug)).
I'd even made freezing opt-out in .configure and wherever else, but while dry-* is a bunch of separate gems, in reality it's quite tightly coupled (e. g. dry-validation pokes dry-schema @private apis) codebase that couldn't be changed independently, so it's probably wishful thinking.

ojab · 2021-09-13T10:34:58Z

😿

ojab · 2021-12-03T21:09:25Z

😿

solnic · 2021-12-06T10:10:24Z

😿

We'll address this eventually. I'm just going back to OSS after a 3+ months break.

timriley

Apologies for the delay on our end, @ojab.

Overall I'm happy to see this go in. I think we should try and exercise this as much as we can between now and 1.0 to make sure we're happy with the API and behaviour. Left one small request for you, @ojab, if you don't mind :)

lib/dry/configurable/setting.rb

timriley · 2022-01-07T03:56:11Z

@solnic @flash-gordon Final check: we all good to merge this? I'm good with it, and given my other recent work here, I can double check this in all our dry-rb projects and then roll it into a release sometime next week.

Fixes dry-rb#104

timriley · 2022-01-07T05:27:07Z

@ojab Just noticed you pushed some changes, and I should have checked with you too, sorry! Are you OK with the current state of this PR?

ojab · 2022-01-07T06:01:17Z

Yeah, I just rebased since there was a merge conflict and I noticed it after opening/reading your comment in GH UI, no worries.

flash-gordon · 2022-01-08T12:40:27Z

@timriley yeah I'm good with it though I don't remember all the details :)

ojab · 2022-04-20T18:36:45Z

😿

timriley · 2022-04-20T22:56:34Z

@ojab I'll release this in the next few days!

timriley · 2022-04-21T05:14:39Z

@ojab Thank you not only for your patch, but for your incredible patience and persistence here. I'd like to give you a trophy of some kind, but instead of hope you can settle for a new dry-configurable release 😄 Merging this one now, and will check your other PR (#131) next.

timriley · 2022-04-21T07:43:59Z

Released in 0.15.0: https://github.com/dry-rb/dry-configurable/releases/tag/v0.15.0

ojab force-pushed the finalize_recursively branch from 64770f3 to b25effd Compare April 10, 2021 15:01

ojab changed the title ~~Make config#finalize! freeze itself recurively~~ Make onfig#finalize! freeze itself recurively Apr 10, 2021

ojab changed the title ~~Make onfig#finalize! freeze itself recurively~~ Make Config#finalize! freeze itself recurively Apr 10, 2021

ojab marked this pull request as ready for review April 10, 2021 15:04

ojab requested a review from solnic as a code owner April 10, 2021 15:04

solnic reviewed Apr 12, 2021

View reviewed changes

spec/integration/dry/configurable/setting_spec.rb Outdated Show resolved Hide resolved

flash-gordon requested changes Apr 12, 2021

View reviewed changes

ojab force-pushed the finalize_recursively branch from b25effd to 1d8fc13 Compare April 12, 2021 12:01

ojab force-pushed the finalize_recursively branch from 1d8fc13 to e6eef5b Compare April 12, 2021 12:13

timriley reviewed Apr 13, 2021

View reviewed changes

ojab force-pushed the finalize_recursively branch 2 times, most recently from 8ae0eb3 to e32be04 Compare April 17, 2021 14:44

ojab changed the title ~~Make Config#finalize! freeze itself recurively~~ Make Config#finalize! freeze itself recurively and allow freezing values Apr 17, 2021

ojab requested review from solnic, flash-gordon and timriley April 17, 2021 14:46

ojab force-pushed the finalize_recursively branch from e32be04 to ba2ff80 Compare April 17, 2021 14:47

ojab commented Apr 17, 2021

View reviewed changes

ojab force-pushed the finalize_recursively branch from ba2ff80 to 6ba01a4 Compare June 9, 2021 14:43

ojab force-pushed the finalize_recursively branch from 6ba01a4 to a7f885d Compare June 9, 2021 14:48

ojab changed the title ~~Make Config#finalize! freeze itself recurively and allow freezing values~~ Make Config#finalize! finalize itself recurively and allow freezing values Jun 9, 2021

ojab force-pushed the finalize_recursively branch from a7f885d to 14c1cdf Compare June 9, 2021 15:26

solnic reviewed Jun 10, 2021

View reviewed changes

spec/integration/dry/configurable/setting_spec.rb Outdated Show resolved Hide resolved

ojab force-pushed the finalize_recursively branch from 14c1cdf to e45d069 Compare June 10, 2021 09:08

ojab requested a review from solnic June 10, 2021 10:26

flash-gordon approved these changes Jun 15, 2021

View reviewed changes

solnic reviewed Jul 19, 2021

View reviewed changes

ojab requested a review from solnic July 21, 2021 19:09

timriley requested changes Dec 6, 2021

View reviewed changes

lib/dry/configurable/setting.rb Show resolved Hide resolved

ojab force-pushed the finalize_recursively branch from e45d069 to 765fb5b Compare December 6, 2021 12:01

ojab requested a review from timriley December 6, 2021 12:02

ojab added 2 commits January 7, 2022 04:13

Make Config#finalize! finalize itself recurively

94966cb

Add option to freeze values to finalize!

b5126e6

Fixes dry-rb#104

ojab force-pushed the finalize_recursively branch from 765fb5b to b5126e6 Compare January 7, 2022 04:15

solnic approved these changes Jan 10, 2022

View reviewed changes

timriley approved these changes Apr 21, 2022

View reviewed changes

timriley merged commit 44f6e45 into dry-rb:main Apr 21, 2022

Make Config#finalize! finalize itself recurively and allow freezing values #105

Make Config#finalize! finalize itself recurively and allow freezing values #105

Conversation

ojab commented Apr 10, 2021

solnic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojab Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timriley Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojab commented Apr 12, 2021

solnic commented Apr 13, 2021

timriley Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

ojab Apr 17, 2021 • edited Loading

Choose a reason for hiding this comment

ojab commented Apr 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojab Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

ojab commented Jun 9, 2021

ojab commented Jul 14, 2021

solnic left a comment

Choose a reason for hiding this comment

ojab commented Jul 20, 2021 • edited Loading

AMHOL commented Jul 21, 2021

ojab commented Jul 21, 2021

ojab commented Sep 13, 2021

ojab commented Dec 3, 2021

solnic commented Dec 6, 2021

timriley left a comment

Choose a reason for hiding this comment

timriley commented Jan 7, 2022 • edited Loading

timriley commented Jan 7, 2022

ojab commented Jan 7, 2022 • edited Loading

flash-gordon commented Jan 8, 2022

ojab commented Apr 20, 2022

timriley commented Apr 20, 2022

timriley commented Apr 21, 2022

timriley commented Apr 21, 2022

Make `Config#finalize!` finalize itself recurively and allow freezing values #105

Make `Config#finalize!` finalize itself recurively and allow freezing values #105

ojab Apr 12, 2021 •

edited

Loading

timriley Apr 13, 2021 •

edited

Loading

timriley Apr 13, 2021 •

edited

Loading

ojab Apr 17, 2021 •

edited

Loading

ojab Jul 14, 2021 •

edited

Loading

ojab commented Jul 20, 2021 •

edited

Loading

timriley commented Jan 7, 2022 •

edited

Loading

ojab commented Jan 7, 2022 •

edited

Loading