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(settings): change HathorManager to use injected settings #787

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Sep 25, 2023

Motivation

Continuing with the settings-related refactors, this PR updates HathorManager so it starts using injected settings, instead of global. This refactor is necessary for the Feature Activation mandatory signaling implementation.

Acceptance Criteria

  • Add settings attribute to HathorManager, removing the module level settings
  • Move DEFAULT_CAPABILITIES from module level to HathorManager.get_default_capabilities()
  • Update Builder and CliBuilder accordingly

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 self-assigned this Sep 25, 2023
@glevco glevco force-pushed the refactor/hathor-manager-settings branch from 73a4417 to 7782a41 Compare September 25, 2023 22:06
@glevco glevco changed the base branch from master to refactor/verification-service September 25, 2023 22:06
Base automatically changed from refactor/verification-service to master September 26, 2023 11:53
@glevco glevco force-pushed the refactor/hathor-manager-settings branch 3 times, most recently from 3c084df to a008e7e Compare September 28, 2023 15:13
@glevco glevco marked this pull request as ready for review September 28, 2023 15:25
jansegre
jansegre previously approved these changes Sep 28, 2023
hathor/manager.py Outdated Show resolved Hide resolved
msbrogli
msbrogli previously approved these changes Sep 28, 2023
hathor/manager.py Show resolved Hide resolved
@glevco glevco dismissed stale reviews from msbrogli and jansegre via 914dd77 September 28, 2023 15:38
@glevco glevco force-pushed the refactor/hathor-manager-settings branch from 914dd77 to 748afab Compare September 28, 2023 15:49
@glevco glevco force-pushed the refactor/hathor-manager-settings branch from 748afab to 0a9b949 Compare September 29, 2023 03:34
@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (c968aea) 84.64% compared to head (0a9b949) 84.69%.
Report is 2 commits behind head on master.

❗ Current head 0a9b949 differs from pull request most recent head 85a3bfc. Consider uploading reports for the commit 85a3bfc to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #787      +/-   ##
==========================================
+ Coverage   84.64%   84.69%   +0.04%     
==========================================
  Files         264      264              
  Lines       22024    22023       -1     
  Branches     3367     3368       +1     
==========================================
+ Hits        18643    18653      +10     
+ Misses       2727     2720       -7     
+ Partials      654      650       -4     
Files Coverage Δ
hathor/builder/builder.py 91.02% <ø> (ø)
hathor/builder/cli_builder.py 74.48% <ø> (ø)
hathor/manager.py 81.37% <54.16%> (+0.19%) ⬆️

... and 6 files with indirect coverage changes

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

@glevco glevco force-pushed the refactor/hathor-manager-settings branch from 0a9b949 to c700dc5 Compare October 9, 2023 18:29
@glevco glevco force-pushed the refactor/hathor-manager-settings branch from c700dc5 to 85a3bfc Compare October 9, 2023 18:29
@msbrogli msbrogli merged commit 85a3bfc into master Oct 9, 2023
@msbrogli msbrogli deleted the refactor/hathor-manager-settings branch October 9, 2023 18:30
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