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

Move all modules into root namespace. #868

Merged
merged 6 commits into from
Dec 6, 2022
Merged

Move all modules into root namespace. #868

merged 6 commits into from
Dec 6, 2022

Conversation

bdice
Copy link
Member

@bdice bdice commented Dec 3, 2022

Description

Closes #756, supersedes #862.

Checklist:

@bdice bdice requested review from a team as code owners December 3, 2022 05:33
@bdice bdice requested review from b-butler and iblanco11981870 and removed request for a team December 3, 2022 05:33
@bdice
Copy link
Member Author

bdice commented Dec 3, 2022

@csadorf @vyasr I rebased the changes from #862 into this PR. Feel free to push to this branch directly.

@codecov
Copy link

codecov bot commented Dec 3, 2022

Codecov Report

Merging #868 (de097c8) into master (766c0b7) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #868      +/-   ##
==========================================
- Coverage   86.63%   86.53%   -0.10%     
==========================================
  Files          48       41       -7     
  Lines        4570     4531      -39     
  Branches      901      901              
==========================================
- Hits         3959     3921      -38     
+ Misses        430      429       -1     
  Partials      181      181              
Impacted Files Coverage Δ
signac/__main__.py 77.72% <100.00%> (-0.07%) ⬇️
signac/_dict_manager.py 88.37% <100.00%> (ø)
signac/_search_indexer.py 97.72% <100.00%> (ø)
signac/_utility.py 68.00% <100.00%> (ø)
signac/config.py 90.47% <100.00%> (ø)
signac/diff.py 100.00% <100.00%> (ø)
signac/filterparse.py 89.65% <100.00%> (ø)
signac/h5store.py 91.03% <100.00%> (ø)
signac/hashing.py 100.00% <100.00%> (ø)
signac/import_export.py 77.24% <100.00%> (ø)
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vyasr
Copy link
Contributor

vyasr commented Dec 4, 2022

I've made two changes:

  • I modified the pre-commit config to use a single global excludes pattern for _vendor rather than specifying it separately for each hook.
  • I implemented the first bullet point from my request on #862, moving search_indexer.py, dict_manager.py, and utility.py into an _internal subpackage. I did not move filterparse.py because I remembered that its functionality is leveraged by signac-dashboard, which means that we probably should be importing parse_filter_args into the top-level signac namespace as well. WDYT?

Assuming those changes look OK I'm happy with the current state of this PR. I would be fine with addressing any additional changes (such as the ones I proposed in #862) in follow-ups.

@bdice
Copy link
Member Author

bdice commented Dec 4, 2022

@vyasr I’m not a huge fan of an _internal subpackage. Nothing ties together the contents of that subpackage except API visibility, which feels odd. I’d rather keep things at the top level and start the module names with _. It doesn’t feel too cluttered to me.

The pre-commit change is good! I forgot that top level excludes were possible.

@vyasr
Copy link
Contributor

vyasr commented Dec 4, 2022

@vyasr I’m not a huge fan of an _internal subpackage. Nothing ties together the contents of that subpackage except API visibility, which feels odd. I’d rather keep things at the top level and start the module names with _. It doesn’t feel too cluttered to me.

The pre-commit change is good! I forgot that top level excludes were possible.

Sounds good, I reverted that change and prefixed those modules with underscores.

@bdice
Copy link
Member Author

bdice commented Dec 5, 2022

Thanks @vyasr! I pushed one small tweak to generalize our exclusions. Otherwise I think this PR is ready to go with a reviewer approval.

@bdice bdice self-assigned this Dec 5, 2022
@bdice bdice added this to the v2.0.0 milestone Dec 5, 2022
Copy link
Member

@b-butler b-butler left a comment

Choose a reason for hiding this comment

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

LGTM

@vyasr
Copy link
Contributor

vyasr commented Dec 6, 2022

For posterity, here's what the project structure looks like now:

signac/
├── __init__.py
├── __main__.py
├── _dict_manager.py
├── _search_indexer.py
├── _utility.py
├── _vendor
│   ├── configobj
│   │   ├── LICENSE
│   │   ├── __init__.py
│   │   ├── _version.py
│   │   └── validate.py
│   └── deprecation
│       ├── LICENSE
│       ├── README
│       └── __init__.py
├── config.py
├── diff.py
├── errors.py
├── filterparse.py
├── h5store.py
├── hashing.py
├── import_export.py
├── job.py
├── linked_view.py
├── migration
│   ├── __init__.py
│   ├── v0_to_v1.py
│   └── v1_to_v2.py
├── project.py
├── schema.py
├── sync.py
├── syncutil.py
└── version.py

In the interest of quicker progress, I'm going to go ahead and merge this now. We can make further changes in subsequent PRs, and merging this now should allow us to make those changes in smaller, more self-contained PRs.

@vyasr
Copy link
Contributor

vyasr commented Dec 6, 2022

@bdice the question I had about filterparse still stands. I think since dashboard is using it we should be exposing whatever functionality it needs more publicly than we are now (but that can be done in a subsequent PR).

@vyasr vyasr merged commit 3de5f48 into master Dec 6, 2022
@vyasr vyasr deleted the reorg-modules branch December 6, 2022 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reorganize modules for signac 2.0
4 participants