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

Clean up utility functions. #597

Merged
merged 2 commits into from
Aug 6, 2021
Merged

Clean up utility functions. #597

merged 2 commits into from
Aug 6, 2021

Conversation

bdice
Copy link
Member

@bdice bdice commented Aug 6, 2021

Description

This PR removes unused utility functions and cleans up test coverage of the utility.py module.

Notes:

  • The _query_yes_no function doesn't need test coverage (it's strictly for interactive use on the terminal).
  • The _split_and_print_progress function can probably be simplified and replaced with tqdm, if it is needed at all. The function is used only once, to print info-level logging for updates to the in-memory state point cache. This should be investigated further, but I've made the function private for now.
  • There was an important edge case of _nested_dicts_to_dotted_keys that wasn't being tested, when a value is an empty dict. I added a test that covers that edge case.

Motivation and Context

Cleanup for signac 2.0.

Types of Changes

  • Documentation update
  • Bug fix
  • New feature
  • Breaking change1

1The change breaks (or has the potential to break) existing functionality.

Checklist:

If necessary:

  • I have updated the API documentation as part of the package doc-strings.
  • I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
  • I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

@bdice bdice requested review from a team as code owners August 6, 2021 06:00
@bdice bdice requested review from csadorf and cbkerr August 6, 2021 06:00
@bdice bdice added this to the v2.0.0 milestone Aug 6, 2021
@bdice bdice added the requires-1.x-deprecation Changes for 2.0 that require a deprecation to be added in 1.x. label Aug 6, 2021
@bdice bdice self-assigned this Aug 6, 2021
"const2": {"const3": 0},
"const4": {},
Copy link
Member Author

Choose a reason for hiding this comment

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

This edge case was being missed until I added this test:

elif key is not None:
yield key, t

@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #597 (c25bee2) into next (257c931) will increase coverage by 0.42%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #597      +/-   ##
==========================================
+ Coverage   83.77%   84.20%   +0.42%     
==========================================
  Files          54       54              
  Lines        5535     5502      -33     
  Branches     1021     1014       -7     
==========================================
- Hits         4637     4633       -4     
+ Misses        668      641      -27     
+ Partials      230      228       -2     
Impacted Files Coverage Δ
signac/__main__.py 75.17% <ø> (ø)
signac/contrib/project.py 85.91% <100.00%> (ø)
signac/contrib/utility.py 59.77% <100.00%> (+12.27%) ⬆️
signac/contrib/schema.py 96.24% <0.00%> (+0.75%) ⬆️

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 257c931...c25bee2. Read the comment docs.

@@ -41,7 +41,7 @@
from .indexing import _SignacProjectCrawler
from .job import Job
from .schema import ProjectSchema
from .utility import _mkdir_p, _nested_dicts_to_dotted_keys, split_and_print_progress
from .utility import _mkdir_p, _nested_dicts_to_dotted_keys, _split_and_print_progress
Copy link
Contributor

Choose a reason for hiding this comment

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

If I've had to guess the function was introduced prior to having the tqdm dependency or because there was some kind of threading issue or so with tqdm. But I agree, we should certainly try to replace it with tqdm if possible.

@bdice bdice merged commit a3330ea into next Aug 6, 2021
@bdice bdice deleted the cleanup-utility branch August 6, 2021 16:22
@bdice bdice mentioned this pull request Aug 6, 2021
vyasr pushed a commit that referenced this pull request Nov 9, 2021
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
bdice added a commit that referenced this pull request Dec 5, 2021
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
vyasr pushed a commit that referenced this pull request Dec 25, 2021
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
vyasr pushed a commit that referenced this pull request Jan 4, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
vyasr pushed a commit that referenced this pull request Jan 27, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
vyasr pushed a commit that referenced this pull request Feb 4, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
vyasr pushed a commit that referenced this pull request Feb 21, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
vyasr pushed a commit that referenced this pull request Feb 21, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
vyasr pushed a commit that referenced this pull request Mar 14, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
vyasr pushed a commit that referenced this pull request Apr 19, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
vyasr pushed a commit that referenced this pull request Apr 21, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
vyasr pushed a commit that referenced this pull request May 2, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
bdice added a commit that referenced this pull request Jun 14, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
bdice added a commit that referenced this pull request Aug 1, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
bdice added a commit that referenced this pull request Oct 7, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
bdice added a commit that referenced this pull request Oct 27, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
vyasr pushed a commit that referenced this pull request Oct 30, 2022
* Clean up utility functions.

* Add missing test case for dotted keys function where values are empty dicts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires-1.x-deprecation Changes for 2.0 that require a deprecation to be added in 1.x.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants