chore: ensure Options are initialised with defaults

[pbernays]

I can't see a reason not to load defaults when initialising an Options object (and the comment on line 158/96 suggests it's desired behaviour), so this calls load_defaults during init and makes it a private method.

Correct me if I'm wrong, but I also can't see a reason to keep the unused format_bare flag option given that arbitrary keys are allowed.

Removes the *_OPTIONS_KEYS constants for being redundant.

Tidies up a couple of lines to be clearer.

[drwl]

Hi @pbernays thanks for submitting a PR and apologies on getting around to this.

I'll try to respond to this inline -- and to be clear, I'm happy to have a conversation around this so that we can improvement the gem.

I can't see a reason not to load defaults when initialising an Options object (and the comment on line 158/96 suggests it's desired behaviour), so this calls load_defaults during init and makes it a private method.

To provide some history, this gem is a hard fork from annotate_models. One of the biggest issues in the old gem was that configuration options and state were global. They could be set in multiple places, and were also read from different places.

One of my initial goals was to make it easier to have isolated functions that were easy to unit test. To get that, most functions now take an AnnotateRB::Options as a parameter and read from that, instead of a global variable. Something I found while doing this, was that some of the default options would have unexpected effects on the code because I didn't explicitly set them. See #34

Correct me if I'm wrong, but I also can't see a reason to keep the unused format_bare flag option given that arbitrary keys are allowed.

We can get rid of this flag -- you're right, it's not used. I didn't touch it because I wanted the migration from the old gem to this one to be easy.

Removes the *_OPTIONS_KEYS constants for being redundant.

When I was first doing an audit of the various options, I made these hashes separate so it was clearer what was being used and where. If it doesn't make sense to keep these anymore then it seems fine to remove.

[pbernays]

Hi @pbernays thanks for submitting a PR and apologies on getting around to this.

First off, and this is overdue, I owe you thanks for your gift to open source. I wasn't looking forward to trying to alter the code of the original gem to add the changes in the other PR I put up, but a colleague showed me your 2yo reddit post, and your refactor made it a breeze. So thank you, I appreciate the time and effort you've put in to improve it.

Also, I don't believe you owe an apology either, especially when I've been sloppy and haven't corrected the rubocop nitpicks yet.

I'll try to respond to this inline -- and to be clear, I'm happy to have a conversation around this so that we can improvement the gem.

I'd be happy to be involved in that if you'd like input from collaborators. The tool's always been a set-and-forget kind of deal for me, so I don't have much of a vision for it. But it is very useful and I'd like to contribute to keep it that way, aligned with whatever your plans for it are. Which begs the question, what are your plans?

I can't see a reason not to load defaults when initialising an Options object (and the comment on line 158/96 suggests it's desired behaviour), so this calls load_defaults during init and makes it a private method.

To provide some history, this gem is a hard fork from annotate_models. One of the biggest issues in the old gem was that configuration options and state were global. They could be set in multiple places, and were also read from different places.

One of my initial goals was to make it easier to have isolated functions that were easy to unit test. To get that, most functions now take an AnnotateRB::Options as a parameter and read from that, instead of a global variable. Something I found while doing this, was that some of the default options would have unexpected effects on the code because I didn't explicitly set them. See #34

Thanks for the clarity. It's been a couple of months since I had my head in this code, and I remember having some thoughts about the specs to do with the options, but what they were... ¯\(ツ)/¯. It seems worth removing this change, figuring out what those ideas are to discuss with you, and then follow up if you're in agreement.

Correct me if I'm wrong, but I also can't see a reason to keep the unused format_bare flag option given that arbitrary keys are allowed.

We can get rid of this flag -- you're right, it's not used. I didn't touch it because I wanted the migration from the old gem to this one to be easy.

Makes perfect sense.

Removes the *_OPTIONS_KEYS constants for being redundant.

When I was first doing an audit of the various options, I made these hashes separate so it was clearer what was being used and where. If it doesn't make sense to keep these anymore then it seems fine to remove.

👍

[drwl]

First off, and this is overdue, I owe you thanks for your gift to open source. I wasn't looking forward to trying to alter the code of the original gem to add the changes in the other PR I put up, but a colleague showed me your 2yo reddit post, and your refactor made it a breeze. So thank you, I appreciate the time and effort you've put in to improve it.

Thanks for the kind words :).

I'd be happy to be involved in that if you'd like input from collaborators. The tool's always been a set-and-forget kind of deal for me, so I don't have much of a vision for it.

This has been my experience as well, although with Rails continuously changing and evolving (a good thing), I've found that updates are needed.

But it is very useful and I'd like to contribute to keep it that way, aligned with whatever your plans for it are. Which begs the question, what are your plans?

My plan is just to continue for a tool/gem/library like this to exist. I have found it in my past Rails jobs as an improvement in the Rails developer experience. Any and all help is welcome.

[drwl]

Everything looks good to me -- just 1 question/discussion point

[drwl]

I'd definitely like your thoughts here --

So when I refactored the old gem, I intentionally made Options read only. In the old gem, it was not clear where options were being set as it was a read-writeable global variable, which made it really hard to debug.

However, we are no longer in the old gem and so it might be less of a concern. Thoughts on keeping it the same versus making it writable by delegating to_h?

[pbernays]

That's a good question. Off the top of my head, I don't believe I wanted this to be writable but a convenience to get a copy. I can't see where I've even used it in the PR, so it might have been something I failed to cleanup after debugging in the console. I think your initial intention is good, I can't think of a reason Options should need to change after they've been initialized.