Add specs, refactoring where necessary

[Inkybro]

This is a personal project, with the hope that it might one day be accepted into master.

The project (optimism) needs some level of specs, regardless. I personally think optimism should allow a per-controller level of configuration, where the root Optimism configuration serves as a "default", so I've included that behavior in this branch. The end result of this is backwards-compatible, where one may configure optimism on a "root-level":

Optimism.configure { |c| c.add_class = false }

And then on a more granular level:

class ThingsController < ActionController::Base
  include Optimism

  configure_optimism do |c|
    c.add_class = false
  end
end

Where more granular configurations will take precedence over less-granular configurations.

This branch also includes an initializer template that is copied to config/initializers for some sane defaults.

Also, I'm well aware that this branch flies in the face of @leastbad's stated intentions of minimal complexity. My reasoning for pursuing this is three-fold: I think per-controller configuration is going to be really valuable, almost entirely regardless of the end-user. I think specs are going to be extremely valuable, no matter what. And I think having a more open-ended configuration system will allow for more open-ended features in the future. Ultimately I just love this project, think it has a lot of potential, and think that minimal complexity shouldn't be an obstacle standing in the way of utility.

This is currently a WIP. Feel free to leave feedback.

[leastbad]

I wish that you'd spoken with me about this endeavor before pouring what appears to be a ton of work into it.

Assuming that I just forgot to bolt on a testing framework, why would you further assume I'd allow RSpec anywhere near my codebase?

Why do you conclude that this library needs per-controller configuration? That seems like massive overkill.

Why did you commingle your testing biases with splitting out the configuration? It seems like there's 2-3 different concepts in this PR, making it basically impossible for me to reason about the individual changes without going through every line and unpacking your intention.

You would have far better luck with smaller PRs that explain why, how and what.

[Inkybro]

I wish that you'd spoken with me about this endeavor before pouring what appears to be a ton of work into it.

Assuming that I just forgot to bolt on a testing framework, why would you further assume I'd allow RSpec anywhere near my codebase?

Why do you conclude that this library needs per-controller configuration? That seems like massive overkill.

Why did you commingle your testing biases with splitting out the configuration? It seems like there's 2-3 different concepts in this PR, making it basically impossible for me to reason about the individual changes without going through every line and unpacking your intention.

You would have far better luck with smaller PRs that explain why, how and what.

Fair response, @leastbad. I was hoping it wouldn't come off as all that. Please understand that although not at all new to coding, I am quite new to open-source contribution.

As stated in the original description, this is really more of a personal project, with no obligation placed on you or optimism itself. I don't ask for or expect this to be merged into master, although I'd be happy to spend time making my commits more amenable if some or all of the functionality seems useful.

Addressing some of this issue-by-issue was likely a wiser approach, regardless. I appreciate that feedback. I certainly would've consulted with you more closely if it were something I has my heart set on seeing merged.

To try and answer your questions more specifically:

Why do you conclude that this library needs per-controller configuration? That seems like massive overkill.

It depends on the scope of the gem. As I stated in the description, I know this flies in the face of your stated intention of "minimal complexity". The idea here is that something like this would open the door for more advanced features and more granular control over Optimism. I'm not going to argue that it's necessary, because it very well may not be, but it seemed fun to try and implement, so I did.

Why did you commingle your testing biases with splitting out the configuration?

I'm not quite sure exactly what you mean by this. With some elaboration, I'd be more than happy to clarify or further admit my own ignorance.

No matter what else, please don't "feel bad" about the work I put in. This was maybe 3 hours of work, and I did it with no expectations... it was more for the journey than anything else. If we can find a way to make it work, that's great. If not, that's fine, too.

Even if you gave this the big old "nay no" and asked me to do a specs branch without my configuration refactoring, I'd be willing to give that a shot.

Also, I'd love to hear why you're so opposed to having used RSpec? I didn't assume you had forgotten anything... I did presume you didn't bother to write tests, since there are no tests. Either way, there is no clarification on this anywhere in the project -- and I assure you I've done my due diligence on the project. You shouldn't expect people to know your plans for testing, if you've added no tests nor plans for the future of testing.

[leastbad]

Generally, I don't start doing programmatic tests for code that I want to keep simple enough to hold in my head. There's this thing some coders do where they can't help themselves from splitting up logic into functions across multiple files - which you had to notice that I specifically am not doing in this library.

The reason I sound pissy and defensive is that your PR started off with the statement:

The project (optimism) needs some level of specs, regardless.

That's just your opinion, man. You can spend your time bikeshedding your projects if you want. Optimism works great for the cases I designed it to work for.

When I'm testing the few changes I make to the library, I test it live on the cases it was created to solve. If I wanted to start building up a more sophisticated set of building blocks, I'd stick to asserts. RSpec provides no value to me.

As to your question: you submitted a single PR out of nowhere that is 4x the size of the current library and intermingles major changes to the configuration with the introduction of an opinionated testing famework. When this kind of thing shows up and you're barely able to keep on top of the slow trickle of tiny requests that comes in already, it feels like someone decided to volunteer your favourite room for a reality television design challenge.

[Inkybro]

Once again, we come to the scope of the project. I've done my best to be mindful and considerate of your intentions with this project. For the third or fourth time, you're responding to my own personal fork. I pushed a commit to my personal fork. You have no obligation here, dude.

Maybe "testing it in my own environment" is good enough for you, and if so, good for you. Maybe your intention wasn't for optimism to be used in production environments that aren't yours. That's fine, but if you're going to take the standoffish approach you seem to have a tendency to take, I think it'd be very kind of you to share that up front in the readme rather than waiting for people to submit issues and code.

[leastbad]

You submitted a PR, not a commit to your personal fork... maybe that's why you are surprised that I'm caught off guard by a patch which proposes to entirely re-write my hard work based on entirely different assumptions.

If you look at the many PRs which I have gratefully accepted, they've all been small and in the spirit of my stated intent, which is documented with thousands of words.

[Inkybro]

Correction: I pushed to my fork and GitHub automatically created a PR. I have no control over that.

[leastbad]

GitHub did no such thing.

[Inkybro]

Well someone other than I, did.

[leastbad]

It was you.

[Inkybro]

sigh Yes, I'm sitting here lying to you, dude, after I spent my time writing a PR for class-wrapped attributes, and then spent my time doing this for myself.

Yep, makes sense. Just lying to you about such a senseless, meaningless, petty thing.

I pushed the commits to my fork, and it opened a PR. Maybe it is something you haven't experienced? I have no idea. Not my job to figure that out man.

[leastbad]

I'm trying really hard to give you the benefit of the doubt. It's clear that you're new to this, and I said as much. I accept that you didn't realize that filing a PR - which is a proposal to make changes - would be something I'd feel compelled to respond to at face value.

I'm also empathetic to the fact that you don't remember or didn't realize taking the step to make it a PR. However, it's simply not something that happens automatically. Instead of blaming it on ghosts, just know that you're always free to make your own forks but if you push a giant PR to the upstream without warning, people are likely to feel like you got invited to a dinner party and decided it would be cool to bring your coworkers.

[Inkybro]

Hang on, you aren't getting me...

Just for the shits and giggles, I'll demonstrate.

[Inkybro]

Do you see it now?

[leastbad]

Do I see what?

[Inkybro]

Could it be that I made a PR on my own fork, and it made its way here?

[leastbad]

Still the same 3 PRs outstanding on this project, friend.

https://github.com/leastbad/optimism/pulls

[Inkybro]

I think I fucked up, I'm so sorry.

Should've listened ;)

[Inkybro]

Thanks man

[leastbad]

It's okay. I really feel badly about the way this conversation has gone.

I am not likely to accept a PR for RSpec, it's true.

However, I'm curious to hear more about what scenarios might call for having different configurations per use inside of one project. Could you talk through what those settings might look like in different cases in one of your apps?

[Inkybro]

First of all: Me too! FUCK! I feel like shit! I'm sorry!!!!

[Inkybro]

Me, personally, no. Not "literally right now," but I could imagine a world in the very near future where I need to set class name(s), at a minimum.

[Inkybro]

As in, within 1-2 months.

[Inkybro]

It may not be sensible to set root Optimism config. In default code, it is included into ActiveController::Base. That seems like solid default behavior.

I can't imagine a reason not to drip other behavior in, carefully?

[Inkybro]

I think that if we had a more robust configuration system, I may be able to handle issues like #19

[leastbad]

Let's hit reset, then. I'm a reasonable human when I'm not struggling under the weight of depression.

I appreciate you being honest wrt use cases. Part of my job as steward is to start with why, then discuss how. If you can't describe why the flexibility is needed, and nobody else has asked for it, then it's my job to say "no, thank you" until such a time as you can lay out some interesting cases which justify added complexity.

Feel free to ponder it and get back to me!

[Inkybro]

First of all, feel better.

19 is a really contrived problem. I think if someone paid me $10,000 to fix it, I'd get it done in about 3 days.

Man, just forget all this. Get better, first.

No hard feelings. You want a coder to talk to? Hit me up man :)

Started with QBASIC, 1997.

[Inkybro]

@leastbad is there a way for you to remove it from project PR? Or do I need to do something? I apologize!

[leastbad]

Oh, that's not necessary! It's not doing any harm, and I am now fully aware of the context in which it was created.

All good.

[Inkybro]

You're very much appreciated, @leastbad! Yours is one of the most ingenious rubygems I've seen in quite a while. Bygones will be bygones.