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

Settings proof of concept with a yaml file #163

Closed
wants to merge 4 commits into from

Conversation

octopusinvitro
Copy link
Contributor

WIP

  • Explain why this pull request is needed
  • Remember to check if the README needs to be updated after this change.
  • Remember to check that all commits pass the tests with git rebase -i --exec 'bundle exec rake' master

Related issues

Closes #104

This gem is added so that we can pass configutarion settings to the
site using a yaml file
We use mySociety's standard way of adding configuration data to
our sites using a yaml file. In this commit, only simple settings
are added.

At the moment, if the requirements change, we have to change the
signature of classes. As the application keeps growing, adding
more arguments to their constructors may not be a good idea.

This site specific settings separates everything that is site
specific from the site, and also can be helpful if we pass it as
an argument to classes as well, so that we can simplify the
signature of their constructors and avoid changes from cascading
the codebase when more settings are needed due to a change in
requirements.
This commit shows how to use the site specific settings in our main
classes. The benefits of passing any site specific setting like
base url, folder names etc. like this is that we don't have to
pollute the constructor with pointless arguments and we screen
the application from internals of the class that can be handled
inside of it.

If we change the configuration or if the class needs any other
setting, that matters only to the class. This approach helps with
that.

Since Sinatra settings are not accessible from tests (and even if
they were, we want to keep these classes independent and uncoupled
from the framework) a method was added in the test helper to read
the settings from disk using yaml
@coveralls
Copy link

Coverage Status

Coverage decreased (-17.7%) to 82.336% when pulling f506841 on settings-proof-yaml into 1dcad52 on master.

@EiEResearch EiEResearch closed this Sep 3, 2019
@EiEResearch EiEResearch deleted the settings-proof-yaml branch September 3, 2019 08:11
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.

Create a config file and move all site-specific stuff from the app
3 participants