[WIP] Refactor & optimize ENV accessors

[marshall-lee]

No description provided.

[e2]

Great changes!

I wouldn't mind releasing a 1.0.0 when you're done. (Just make sure to ask).

Things I don't like about the API (but I don't mind enough to rethink):

• I don't like the stubbing helpers much (builder vs class)
• the .instance.create_method probably isn't a good choice of words
• the AutoEnvironment part (naming and how it's used)

[marshall-lee]

@e2 Please look at the latest commit. I refactored specs a bit. Stubs are removed because they're not useful. There's no reason to stub Loader and Dumper classes since they have no side effects. Common behavior in specs with/without namespace is extracted to shared examples.

[marshall-lee]

Also I don't think that stubbing new method of Loader and Dumper classes was a good idea.
Stubs are useful when instances are passed as a dependencies. Loader and Dumper instances are not injectable so I treat them as internal so they should not be mentioned in Environment specs at all.

[marshall-lee]

I also decided that it's better to isolate ENV side effect with just a regular Hash instance which is recreated before each example and stub_const is not a bad tool for this.

double_instance(Hash) (as isolated ENV) and its further usage (allow(ENV).to receive(:[]=) ...) look ugly to reader. I think it's more natural to write a straightforward ENV['key'] = 123 in specs and see what's happening in Nenv.

[marshall-lee]

Maybe pass ENV as a dependency to Nenv::Environment?

[e2]

they have no side effects

Yes. They only exist for the sake of refactoring out some logic. And readability.

Loader and Dumper instances are not injectable so I treat them as internal

Yes. At the time I wondered if anyone would want to change their behavior, but I think the dumping/loading defaults are pretty good as is. Classes could be named DefaultDumper and DefaultLoader to reflect this.

On second thoughts, those classes pretty much just represent a "default block". So instead of using those classes directly, it may be better to just have a default loader and dumper block (if none are supplied).

Loaders/dumpers are usually method specific, not instance specific.

I also don't think dumper/loader classes should check/handle the existence of the callback method. No reason to even instantiate the class if there is a callback.

it's better to isolate ENV side effect with just a regular Hash

Good idea. The only thing I'd do is make sure the behavior is accurate, e.g.

ENV['key'] = 123
TypeError: no implicit conversion of Fixnum into String

Maybe pass ENV as a dependency to Nenv::Environment?

That would be cool! That way Nenv could be used for adding convenience methods to any Hash. Of course upcase when sanitizing wouldn't make sense for arbitrary hashes: https://github.com/e2/nenv/blob/master/lib/nenv/environment.rb#L32

[marshall-lee]

That would be cool! That way Nenv could be used for adding convenience methods to any Hash. Of course upcase when sanitizing wouldn't make sense for arbitrary hashes

Hmm, I don't like it's a good idea anymore. Yes, because of sanitizing and because of weak namespace conventions. Implementing something generic here would be an overkill and re-inventing of OpenStruct :)

[marshall-lee]

Yes. At the time I wondered if anyone would want to change their behavior, but I think the dumping/loading defaults are pretty good as is. Classes could be named DefaultDumper and DefaultLoader to reflect this.

On second thoughts, those classes pretty much just represent a "default block". So instead of using those classes directly, it may be better to just have a default loader and dumper block (if none are supplied).

Loaders/dumpers are usually method specific, not instance specific.

I also don't think dumper/loader classes should check/handle the existence of the callback method. No reason to even instantiate the class if there is a callback.

Please look at last commit, I think now we got it right!

[e2]

Hmm, I don't like it's a good idea anymore. Yes, because of sanitizing and because of weak namespace conventions. Implementing something generic here would be an overkill and re-inventing of OpenStruct :)

I agree about it being an overkill compared to OpenStruct.

Still, I think having ENV itself hardcoded inside the class is a bad idea.

ENV simply isn't versatile - it's a globally accessible constant representing a system resource. And since people will likely use Nenv just like ENV, they'll have a hard time "injecting" any behavior.

E.g. what if someone wants to log all Nenv related calls to ENV? Rewriting loader/dumper blocks may not be convenient, especially with how Nenv combines/splits keys and namespaces. The solution would be to supply an ENV wrapper to Nenv. E.g. Through Nenv.env_class= method.

But that's probably for another PR ...

Anyway, the changes are beautiful, so I'm merging them. I can release this as 1.0, unless you have objections.

Thanks!

[marshall-lee]

@e2 there were no breaking changes, so maybe release just next minor?

[e2]

I released 0.3.0.

I thought about releasing 1.0.0 so people can lock to the major version, while API hasn't changed in over a year.