-
Notifications
You must be signed in to change notification settings - Fork 111
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
Move IdentityConfig code into Identity::Hostdata #10476
Conversation
Most of the changes are automated, but this touches the entire codebase, so merge conflicts are kind of inevitable, my strategy is going to be to:
|
- Allows for better sharing across codebases - Keeps "IdentityConfig.store" around to forward, which allows keeping old syntax for now changelog: Internal, Source code, Use shared logic for accessing config data
897f3ee
to
72a4478
Compare
def self.store | ||
Identity::Hostdata.config | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this shorthand and redid the PR to minimize diff noise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
|
||
config.asset_sources = AssetSources.new( | ||
manifest_path: Rails.public_path.join('packs', 'manifest.json'), | ||
cache_manifest: Rails.env.production? || Rails.env.test?, | ||
i18n_locales: IdentityConfig.store.available_locales, | ||
i18n_locales: Identity::Hostdata.config.available_locales, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is now basically the only file that's been migrated to the new syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i didn't know much about this code, but i found this pretty easy to follow. lgtm, with one small non-blocking thought.
def self.store | ||
Identity::Hostdata.config | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
config/application.rb
Outdated
@@ -32,24 +32,24 @@ class Application < Rails::Application | |||
Identity::Hostdata.logger.level = log_level | |||
end | |||
|
|||
configuration = Identity::Hostdata::ConfigReader.new( | |||
Identity::Hostdata.load_config!( | |||
app_root: Rails.root, | |||
logger: Identity::Hostdata.logger, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a big deal, but you could skip passing in the logger value as it evokes as
logger: logger || self.logger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah great reminder! updated in 7d3ed90
Co-authored-by: Davida Marion <sgtplt@users.noreply.github.com>
See 18F/identity-hostdata#39
Big rename of
IdentityConfig.store
toIdentity::Hostdata.config
Lets us share code with other repos, as this code has been copy-pasted quite a few times now