Skip to content

refactor(settings): make HathorSettings always return the same instance#626

Merged
jansegre merged 1 commit intomasterfrom
refactor/settings-singleton
May 24, 2023
Merged

refactor(settings): make HathorSettings always return the same instance#626
jansegre merged 1 commit intomasterfrom
refactor/settings-singleton

Conversation

@jansegre
Copy link
Member

This wasn't exactly the case before this commit. On one side when using the deprecated module settings the same module would always be loaded, and the same SETTINGS property would be accessed, but reloading the module (or at least leaving that up to importlib) unnecessary. On the new YAML system the file is reloaded everytime, this is also unnecessary.

This commit will make it the last HathorSettings instantiated will be returned, as long as the source is the same, otherwise an exception is raised.

There is a little small caveat that is fixed in this commit is that in the YAML file could in theory be altered between different calls to HathorSettings, which would lead to different content being loaded silently. What happens now is that the first time it is loaded is what is used for every call, any change in the file won't have any effect.

Acceptance Criteria

  • always return the same hathor.conf.HathorSettings instance when calling hathor.conf.get_settings.HathorSettings
  • replace get_settings_module with get_settings_module_path that returns the loaded path instead of the loaded module

@jansegre jansegre requested a review from glevco May 23, 2023 18:53
@jansegre jansegre requested a review from msbrogli as a code owner May 23, 2023 18:53
@jansegre jansegre self-assigned this May 23, 2023
@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #626 (94e99cc) into master (7f35116) will increase coverage by 0.02%.
The diff coverage is 65.71%.

@@            Coverage Diff             @@
##           master     #626      +/-   ##
==========================================
+ Coverage   83.91%   83.94%   +0.02%     
==========================================
  Files         240      240              
  Lines       20127    20126       -1     
  Branches     2744     2743       -1     
==========================================
+ Hits        16890    16895       +5     
+ Misses       2640     2635       -5     
+ Partials      597      596       -1     
Impacted Files Coverage Δ
hathor/conf/get_settings.py 70.00% <63.63%> (+15.00%) ⬆️
hathor/builder/cli_builder.py 70.74% <100.00%> (-0.16%) ⬇️

... and 4 files with indirect coverage changes

@jansegre jansegre force-pushed the refactor/settings-singleton branch from 5e7f3d9 to 4fe59cd Compare May 23, 2023 23:23
msbrogli
msbrogli previously approved these changes May 24, 2023
This wasn't _exactly_ the case before this commit. On one side when
using the deprecated module settings the same module would always be
loaded, and the same `SETTINGS` property would be accessed, but
reloading the module (or at least leaving that up to importlib)
unnecessary. On the new YAML system the file is reloaded everytime, this
is also unnecessary.

This commit will make it the last HathorSettings instantiated will be
returned, as long as the source is the same, otherwise an exception is
raised.

There is a little small caveat that is fixed in this commit is that in
the YAML file could in theory be altered between different calls to
HathorSettings, which would lead to different content being loaded
silently. What happens now is that the first time it is loaded is what
is used for every call, any change in the file won't have any effect.
@jansegre jansegre force-pushed the refactor/settings-singleton branch from 4db657c to 94e99cc Compare May 24, 2023 18:06
@jansegre jansegre merged commit 45af368 into master May 24, 2023
@jansegre jansegre deleted the refactor/settings-singleton branch May 24, 2023 20:46
@jansegre jansegre mentioned this pull request Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants