Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor flow's config to use a minimal amount of signac's config module #649

Merged
merged 12 commits into from
Jul 11, 2022

Conversation

b-butler
Copy link
Member

@b-butler b-butler commented Jun 2, 2022

Description

  • Remove flow.util.config.get_config_value. Removes an extant template filter.
  • Make flow.util.config.require_config_value take a project.
  • Rename flow.util.config.require_config_value to flow.util.config.get_config_value. This could potential trip users up if they use our template filter as now the behavior changes.
  • Remove flow.environment.ComputeEnvironment.get_config_value. Use the project to get environment specific configuration values.
  • Remove use of signac.common.config.read_config_file and load_config everywhere.

We no longer depend at all on signac.common.config.

Motivation and Context

signac is significantly reducing its Python config API, and this prepares for that change.

Checklist:

@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #649 (3b66da7) into master (10bbcb4) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
+ Coverage   79.81%   79.85%   +0.04%     
==========================================
  Files          33       33              
  Lines        3195     3187       -8     
  Branches      681      681              
==========================================
- Hits         2550     2545       -5     
+ Misses        507      505       -2     
+ Partials      138      137       -1     
Impacted Files Coverage Δ
flow/environment.py 78.32% <ø> (-0.59%) ⬇️
flow/project.py 81.72% <ø> (-0.02%) ⬇️
flow/util/config.py 100.00% <100.00%> (+15.00%) ⬆️
flow/util/template_filters.py 68.83% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10bbcb4...3b66da7. Read the comment docs.

b-butler added 3 commits July 6, 2022 14:19
This allows the sharing of some useful base classes to multiple test
files.
Creates a test file for template filters
@b-butler b-butler marked this pull request as ready for review July 7, 2022 15:54
@b-butler b-butler requested review from a team as code owners July 7, 2022 15:54
@b-butler b-butler requested review from csadorf, shihkual and vyasr and removed request for a team and csadorf July 7, 2022 15:54
Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config changes look great. I have a few comments on the changes in conftest.py (which seem unnecessary to me) but I'm approving the core of the config-related changes.

b-butler and others added 2 commits July 7, 2022 12:38
@bdice
Copy link
Member

bdice commented Jul 8, 2022

@b-butler Please update the changelog and then this can be merged.

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This LGTM

Copy link

@shihkual shihkual left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good. Thanks!

@b-butler b-butler merged commit fc8f50b into master Jul 11, 2022
@b-butler b-butler deleted the refactor/config branch July 11, 2022 17:41
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants