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

Add function to initialize a test workspace #215

Merged
merged 12 commits into from
Aug 9, 2019
Merged

Conversation

tcmoore3
Copy link
Member

@tcmoore3 tcmoore3 commented Jul 26, 2019

This is related to signac-flow issue glotzerlab/signac-flow#130 and glotzerlab/signac-flow#129, where we decided it makes more sense to break up the data space initialization into signac core and the flow project initialization into flow.

Types of Changes

  • New feature

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.

This is related to signac-flow issue #129 and PR #130, where we decided
it makes more sense to break up the data space initialization into
signac core and the flow project initialization into flow.
@tcmoore3 tcmoore3 requested review from csadorf and a team as code owners July 26, 2019 15:46
@codecov
Copy link

codecov bot commented Jul 26, 2019

Codecov Report

Merging #215 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #215      +/-   ##
==========================================
+ Coverage   65.15%   65.25%   +0.09%     
==========================================
  Files          37       38       +1     
  Lines        5599     5615      +16     
==========================================
+ Hits         3648     3664      +16     
  Misses       1951     1951
Impacted Files Coverage Δ
signac/testing.py 100% <100%> (ø)
signac/__init__.py 100% <100%> (ø) ⬆️

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 add19c3...5a4b389. Read the comment docs.

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

I'm going to add a few unit tests and then this is ready to ship.

signac/testing.py Outdated Show resolved Hide resolved
signac/testing.py Show resolved Hide resolved
signac/testing.py Outdated Show resolved Hide resolved
@csadorf
Copy link
Contributor

csadorf commented Jul 27, 2019

@tcmoore3 I've added a few comments with suggestions to keep track of things. I'm happy to take those on myself unless you want to do that. Just let me know.

@csadorf csadorf added the enhancement New feature or request label Jul 27, 2019
Changes to address comments in PR #215
@tcmoore3
Copy link
Member Author

tcmoore3 commented Jul 30, 2019

@csadorf Cool. I made the suggested changes. Did you already write unit tests? I didn't see them on the branch.

@tcmoore3 tcmoore3 requested review from csadorf and vyasr August 2, 2019 15:16
@csadorf
Copy link
Contributor

csadorf commented Aug 2, 2019

@csadorf Cool. I made the suggested changes. Did you already write unit tests? I didn't see them on the branch.

No, I didn't get to it. Maybe you or someone else could write them?

@csadorf csadorf requested review from atravitz and removed request for csadorf August 6, 2019 14:17
@csadorf csadorf removed the request for review from vyasr August 8, 2019 17:10
Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

I'd like to extend the unit test just a little bit prior to merging, otherwise this is all good.

tests/test_project.py Outdated Show resolved Hide resolved
tcmoore3 and others added 4 commits August 8, 2019 20:45
Returning the jobs makes more sense, because we already have a reference
to the project and this makes it easier to delete those jobs again.
@csadorf
Copy link
Contributor

csadorf commented Aug 9, 2019

I am going to try to fix issue #217 before merging this.

Copy link
Member Author

@tcmoore3 tcmoore3 left a comment

Choose a reason for hiding this comment

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

LGTM

tests/test_project.py Outdated Show resolved Hide resolved
@csadorf csadorf merged commit 0345c15 into master Aug 9, 2019
@csadorf csadorf deleted the feature/test-project branch August 9, 2019 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants