Skip to content

refactor: move get settings [part 1]#768

Merged
glevco merged 1 commit intomasterfrom
refactor/move-get-settings-1
Sep 12, 2023
Merged

refactor: move get settings [part 1]#768
glevco merged 1 commit intomasterfrom
refactor/move-get-settings-1

Conversation

@glevco
Copy link
Contributor

@glevco glevco commented Sep 2, 2023

Motivation

This PR is the first on of a series of PRs focused on improving settings handling in the project, aiming to remove "global settings" that often cause importing order problems and limit varying settings for tests.

Those improvements will be made in two phases:

  1. Renaming the HathorSettings() function to get_settings() and removing all invocations from module-level, moving them to inside functions/methods. This will be done incrementally in multiple PRs, and this PR is part 1 of that.
  2. Changing all usages of settings to use injection instead of direct access.

Eventually the objective is that get_settings() is only called once in the whole system, and then injected as arguments on all objects/functions that need it.

In this PR, changes are very simple, code is mostly just renamed and the function call is moved. No logic should be changed. The only exceptions are changes to conform to linters and such.

Acceptance Criteria

  • Create new get_settings() function that is just a forward of HathorSettings(). This is just so we can do an "incremental rename" without having to touch all code that uses HathorSettings() at once. When it's done, HathorSettings() will be removed.
  • Remove some usages of HathorSettings() from the module level and move it to inside functions/methods
    • For functions, the function just calls the new get_settings()
    • For classes we set self._settings = get_settings() in the class' __init__(), and update all methods to use that new attribute

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@glevco glevco added the refactor label Sep 2, 2023
@glevco glevco self-assigned this Sep 2, 2023
@glevco glevco force-pushed the refactor/move-get-settings-1 branch 2 times, most recently from bb9b38c to 126309d Compare September 2, 2023 20:24
@glevco glevco mentioned this pull request Sep 2, 2023
1 task
@codecov
Copy link

codecov bot commented Sep 4, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.04% ⚠️

Comparison is base (2ebb62f) 84.74% compared to head (2404c82) 84.71%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #768      +/-   ##
==========================================
- Coverage   84.74%   84.71%   -0.04%     
==========================================
  Files         259      259              
  Lines       21940    21933       -7     
  Branches     2964     2964              
==========================================
- Hits        18594    18580      -14     
- Misses       2700     2708       +8     
+ Partials      646      645       -1     
Files Changed Coverage Δ
hathor/simulator/fake_connection.py 81.62% <ø> (-1.81%) ⬇️
hathor/transaction/__init__.py 100.00% <ø> (ø)
hathor/conf/get_settings.py 71.42% <100.00%> (+1.42%) ⬆️
hathor/p2p/sync_v1/agent.py 85.52% <100.00%> (+0.44%) ⬆️
hathor/p2p/sync_v1/downloader.py 92.02% <100.00%> (+0.04%) ⬆️
hathor/p2p/sync_v2/agent.py 75.83% <100.00%> (-0.88%) ⬇️
hathor/p2p/utils.py 73.87% <100.00%> (ø)
hathor/prometheus.py 89.65% <100.00%> (ø)
hathor/simulator/miner/geometric_miner.py 100.00% <100.00%> (ø)
hathor/simulator/simulator.py 94.64% <100.00%> (ø)
... and 6 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@glevco glevco marked this pull request as ready for review September 4, 2023 18:58
@glevco glevco mentioned this pull request Sep 5, 2023
1 task
jansegre
jansegre previously approved these changes Sep 5, 2023
msbrogli
msbrogli previously approved these changes Sep 6, 2023
@glevco glevco dismissed stale reviews from msbrogli and jansegre via 6cf7be9 September 11, 2023 00:14
@glevco glevco force-pushed the refactor/move-get-settings-1 branch 2 times, most recently from def3c1c to 5837030 Compare September 11, 2023 15:46
@glevco glevco force-pushed the refactor/move-get-settings-1 branch from d6e19ea to ac0a152 Compare September 11, 2023 19:26
@glevco glevco force-pushed the refactor/move-get-settings-1 branch from ac0a152 to 2404c82 Compare September 12, 2023 04:19
@glevco glevco merged commit dbeaa9e into master Sep 12, 2023
@glevco glevco deleted the refactor/move-get-settings-1 branch September 12, 2023 20:39
This was referenced Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants