-
Notifications
You must be signed in to change notification settings - Fork 6
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
[WIP] Refactor & optimize ENV accessors #7
Conversation
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):
|
Creepy mocks are removed. Common behavior is extracted to shared examples.
@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 |
Also I don't think that stubbing |
I also decided that it's better to isolate
|
Maybe pass |
Yes. They only exist for the sake of refactoring out some logic. And readability.
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 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.
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
That would be cool! That way Nenv could be used for adding convenience methods to any Hash. Of course |
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 |
Please look at last commit, I think now we got it right! |
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 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! |
@e2 there were no breaking changes, so maybe release just next minor? |
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. |
No description provided.