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

refactor: move get settings [part 2] #769

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Sep 2, 2023

Depends on #768

Motivation

Please read the description on #768, this PR is just a follow-up part.

Acceptance Criteria

  • 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-2 branch from 8761ff5 to 5274b13 Compare September 4, 2023 19:25
@glevco glevco marked this pull request as ready for review September 4, 2023 19:40
@jansegre
Copy link
Member

jansegre commented Sep 5, 2023

If this refactor is complete and there aren't any old import patterns I recommend adding a linter to extras/custom_checks.sh to prevent us from accidentally using a deprecated pattern.

NOTE: GitHub seems unstable (I've got a 500 error a few times trying to add this comment to an approval)

@glevco
Copy link
Contributor Author

glevco commented Sep 5, 2023

If this refactor is complete and there aren't any old import patterns I recommend adding a linter to extras/custom_checks.sh to prevent us from accidentally using a deprecated pattern.

That's a good idea, I'll make sure to do that, but it can't be done yet. There are still a lot of places to change, it'll take some other PRs to kill all usages. I'm beginning with the easy ones, but then there will be a batch of "hard" ones that have to change a bit more of logic (for example, code that uses settings to define default values for classes, etc)

@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Patch coverage: 90.62% and project coverage change: -0.05% ⚠️

Comparison is base (dbeaa9e) 84.75% compared to head (d8d5435) 84.71%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #769      +/-   ##
==========================================
- Coverage   84.75%   84.71%   -0.05%     
==========================================
  Files         259      259              
  Lines       21933    21931       -2     
  Branches     2964     2964              
==========================================
- Hits        18590    18579      -11     
- Misses       2700     2706       +6     
- Partials      643      646       +3     
Files Changed Coverage Δ
hathor/consensus/context.py 100.00% <ø> (ø)
hathor/indexes/rocksdb_height_index.py 94.11% <ø> (-0.12%) ⬇️
hathor/merged_mining/coordinator.py 34.02% <ø> (-0.19%) ⬇️
hathor/mining/ws.py 50.00% <ø> (-0.71%) ⬇️
hathor/consensus/block_consensus.py 94.36% <57.14%> (ø)
hathor/graphviz.py 77.02% <66.66%> (ø)
hathor/consensus/consensus.py 97.89% <83.33%> (ø)
hathor/consensus/transaction_consensus.py 96.11% <88.88%> (ø)
hathor/builder/builder.py 91.79% <100.00%> (ø)
hathor/builder/cli_builder.py 74.22% <100.00%> (-0.14%) ⬇️
... and 7 more

... and 4 files with indirect coverage changes

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

msbrogli
msbrogli previously approved these changes Sep 6, 2023
hathor/crypto/util.py Outdated Show resolved Hide resolved
hathor/crypto/util.py Outdated Show resolved Hide resolved
hathor/crypto/util.py Outdated Show resolved Hide resolved
jansegre
jansegre previously approved these changes Sep 8, 2023
@glevco glevco force-pushed the refactor/move-get-settings-1 branch from 6cf7be9 to def3c1c Compare September 11, 2023 00:15
@glevco glevco force-pushed the refactor/move-get-settings-2 branch 2 times, most recently from 599cc4f to a6a965b Compare September 11, 2023 00:33
@glevco glevco force-pushed the refactor/move-get-settings-1 branch from def3c1c to 5837030 Compare September 11, 2023 15:46
@glevco glevco force-pushed the refactor/move-get-settings-2 branch 2 times, most recently from 845bf0f to d08e2fc Compare September 11, 2023 16:08
@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-2 branch from d08e2fc to 7989b9d Compare September 11, 2023 19:35
@glevco glevco force-pushed the refactor/move-get-settings-1 branch from ac0a152 to 2404c82 Compare September 12, 2023 04:19
@glevco glevco force-pushed the refactor/move-get-settings-2 branch from 7989b9d to ea1fe51 Compare September 12, 2023 04:20
Base automatically changed from refactor/move-get-settings-1 to master September 12, 2023 20:39
@glevco glevco dismissed stale reviews from jansegre and msbrogli September 12, 2023 20:39

The base branch was changed.

@glevco glevco force-pushed the refactor/move-get-settings-2 branch from ea1fe51 to d8d5435 Compare September 12, 2023 20:40
@glevco glevco merged commit 87ae52e into master Sep 19, 2023
@glevco glevco deleted the refactor/move-get-settings-2 branch September 19, 2023 16:56
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