Skip to content

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented Nov 12, 2021

A nod towards DI and to ease extending the actual class, this PR adds a parameter for the Config file to be used to make handler definition easier. I think ideally this parameter would be required and the service would handle the config fallback, but that BC will have to wait for version 2.

@MGatner MGatner requested a review from lonnieezell November 12, 2021 15:14
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no problems with this. It's better design. I'll admit I've gotten a little lazy there since you can at any point inject your own version of the config file into the factory, but good call.

@MGatner
Copy link
Member Author

MGatner commented Nov 13, 2021

I'm finding this library difficult to extend, but as of this morning I think it is more a conceptual issue than just code. I think this PR is still a good idea, but I will probably make a paradigm pivot for my own project.

@MGatner MGatner merged commit c7d6c8f into develop Nov 13, 2021
@MGatner MGatner deleted the config-inject branch November 13, 2021 12:41
@lonnieezell
Copy link
Member

I'm guessing you're trying to make it work for per-user settings?

I think that's a good expansion, but you're right - currently it's not setup for that.

I could see:

  1. Making the read/write functionality into a trait
  2. Adding two fields to the database: object_type and object_id or something similar
  3. Making the trait write and semi-automatically knowing the object it's attached to and it's primary key. For general settings, it would be a Settings object with no ID, I imagine.

I can see something like that being quite useful. And am not opposed to a rapid 2.0 release :)

@MGatner
Copy link
Member Author

MGatner commented Nov 13, 2021

I'm well underway on this as a separate library, but if you see that as desirable to the core of this I'd be happy to move it into here. The gist is: a separate set of methods or functions to set/get a user setting, and nonexistent entries fall back on the regular behavior.

@lonnieezell
Copy link
Member

lonnieezell commented Nov 13, 2021 via email

@MGatner MGatner mentioned this pull request Nov 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants