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

Fix packaging script files for CLI #765

Merged
merged 5 commits into from
Sep 19, 2023
Merged

Fix packaging script files for CLI #765

merged 5 commits into from
Sep 19, 2023

Conversation

b-butler
Copy link
Member

@b-butler b-butler commented Sep 15, 2023

Description

Adds the necessary __init__.py file.

Motivation and Context

Version 0.26.0 is broken when installing.

Resolves: #763

Checklist:

@b-butler b-butler requested review from a team as code owners September 15, 2023 22:19
@b-butler b-butler requested review from mikemhenry and pepak13 and removed request for a team September 15, 2023 22:19
@mikemhenry mikemhenry enabled auto-merge (squash) September 15, 2023 22:21
@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #765 (37811aa) into main (bec76bb) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #765      +/-   ##
==========================================
+ Coverage   69.28%   69.33%   +0.05%     
==========================================
  Files          43       44       +1     
  Lines        4294     4295       +1     
  Branches     1047     1047              
==========================================
+ Hits         2975     2978       +3     
+ Misses       1108     1107       -1     
+ Partials      211      210       -1     
Files Changed Coverage Δ
flow/scripts/__init__.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

changelog.txt Outdated Show resolved Hide resolved
changelog.txt Outdated Show resolved Hide resolved
changelog.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

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

Looks like a merge got botched

@b-butler
Copy link
Member Author

@mikemhenry thanks for the catch.

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.

Would like to request tests before merging.

# Copyright (c) 2023 The Regents of the University of Michigan
# All rights reserved.
# This software is licensed under the BSD 3-Clause License.
"""Import command line scripts."""
Copy link
Member

Choose a reason for hiding this comment

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

Can we add very basic CLI tests? Even just flow --help. See signac for an example of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have these tests see https://github.com/glotzerlab/signac-flow/blob/main/tests/test_shell.py.

My guess is that tests are run in the repo directory which has the file but the install doesn't due to setuptools package detection requiring an __init__.py.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I'll approve this.

Copy link
Member

Choose a reason for hiding this comment

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

This is why I run tests on installed packages without any source directory. Too many issues like this can occur.

@b-butler b-butler requested a review from bdice September 19, 2023 14:15
@bdice bdice dismissed mikemhenry’s stale review September 19, 2023 14:17

Merge artifact has been fixed.

@b-butler b-butler merged commit c55c861 into main Sep 19, 2023
12 checks passed
@b-butler b-butler deleted the fix/package-scripts branch September 19, 2023 14:30
@b-butler b-butler added this to the v0.26.1 milestone Sep 19, 2023
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.

4 participants