Skip to content
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

Configuration cleanup #378

Closed
wants to merge 3 commits into from
Closed

Configuration cleanup #378

wants to merge 3 commits into from

Conversation

Roemer
Copy link
Collaborator

@Roemer Roemer commented Jan 11, 2016

The configuration has interfaces but almost everywhere are the concrete classes being used.
I'd like to remove all the singletons from the configuration classes and propose to use a Locator where the concrete class can be overwritten.
In this PR in it's current state you see this implementend with the WebBrowserConfiguration.
Please give me feedback on what you think before I do the same with ScreenObjecs configuration and Core und UIItems configuration.

@JakeGinnivan
Copy link
Member

Playing devils advocate. Why do we even need the interfaces?

Let's make configuration static and remove the interfaces entirely if we are going to make the breaking change?

@JakeGinnivan
Copy link
Member

propose to use a Locator where the concrete class can be overwritten

What does that give us?

@Roemer
Copy link
Collaborator Author

Roemer commented Jan 11, 2016

I like the possibility to have different configuration sources. By default, White provides the one from the App.config. But someone might want to use configurations in a textfile or a database or dynamically generated.
We could also think about having the configuration class static with just the value getters/setters and then make configuration readers with interfaces and provide the appconfigreader which fills the static class there.

@Roemer
Copy link
Collaborator Author

Roemer commented Jan 11, 2016

propose to use a Locator where the concrete class can be overwritten

What does that give us?

Static access to a class of a certain interface without exactly knowing the class.

@Roemer
Copy link
Collaborator Author

Roemer commented Jan 11, 2016

I added an alternative version (less interfaces) with RepositoryConfiguration.
This is the configuration instance (independent of how it was read). And then there are ConfigurationReaders (currently only the AppConfigReader) which allows to fill configuration instances with some logic. What do you think about that?

@Roemer
Copy link
Collaborator Author

Roemer commented Jan 11, 2016

Any feedback on the last commit (except the test failures)?

@dpisanu
Copy link
Contributor

dpisanu commented Jan 12, 2016

Looks ok from my point of view.
I like the move to only going by the Interfaces and not the concrete Class Implementations.

@Roemer
Copy link
Collaborator Author

Roemer commented Jan 12, 2016

So you prefer the one in WebBrowserConfiguration? One downside there is that each implementation of IWebBrowserConfiguration has to set it's own default values and have it's own implementation of how it handles the values of the settings.

@dpisanu
Copy link
Contributor

dpisanu commented Jan 12, 2016

it's a question of how much flexibility do you want to provide.
what should be avoided though is giving the tool and flexibility but then doing some default 'infejction' because flexibility causes overhead.
this really really is a decision above my paygrade :p

@Roemer
Copy link
Collaborator Author

Roemer commented Jan 12, 2016

I think the flexibility we need is in how the configuration values are read and not how to get the values once they are read. That's why I'd currently prefer the 2nd version (from the RepositoryConfiguration).

@Roemer
Copy link
Collaborator Author

Roemer commented Jan 12, 2016

This is superseeded with #380

@Roemer Roemer closed this Jan 12, 2016
@Roemer Roemer deleted the configuration-cleanup branch January 12, 2016 15:04
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.

3 participants