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

Feature/access project from job #806

Closed
wants to merge 0 commits into from

Conversation

AlainKadar
Copy link
Contributor

Description

Users can access the project which a job belongs to from the job object

Motivation and Context

Resolves #423

Checklist:

@AlainKadar AlainKadar requested review from a team as code owners August 10, 2022 18:42
@AlainKadar AlainKadar requested review from vyasr and syjlee and removed request for a team August 10, 2022 18:42
@AlainKadar AlainKadar marked this pull request as draft August 10, 2022 18:44
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.

@AlainKadar Can you rebase this on the latest next branch? It looks like there are some extra commits on this branch.

You may need to hard-reset your local copy of the next branch because we've been progressively rebasing it and rewriting history to keep next relatively isolated/clean:

git fetch --all
git checkout next
git reset --hard origin/next
git checkout feature/access-project-from-job
git rebase -i next

Comment on lines 614 to 617
@project.setter
def project(self, new_project):
raise ValueError("Setting project attribute directly is not allowed")

Copy link
Member

Choose a reason for hiding this comment

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

Just delete this -- without a setter, the property will not be settable and will raise an AttributeError.

Suggested change
@project.setter
def project(self, new_project):
raise ValueError("Setting project attribute directly is not allowed")

@@ -238,6 +238,13 @@ def test_deepcopy(self):
copied_job.sp.a = 3
assert copied_job in self.project

def test_project(self):
job = self.project.open_job({"a": 0})
assert job.project.path == self._tmp_pr
Copy link
Member

Choose a reason for hiding this comment

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

Let's test a few more invariants.

Suggested change
assert job.project.path == self._tmp_pr
assert isinstance(job.project, signac.Project)
assert job in job.project
assert job.project.path == self._tmp_pr

@codecov
Copy link

codecov bot commented Aug 10, 2022

Codecov Report

Merging #806 (ebccc86) into next (52ff017) will decrease coverage by 1.27%.
The diff coverage is 78.98%.

❗ Current head ebccc86 differs from pull request most recent head 1d63175. Consider uploading reports for the commit 1d63175 to get more accurate results

@@            Coverage Diff             @@
##             next     #806      +/-   ##
==========================================
- Coverage   86.32%   85.04%   -1.28%     
==========================================
  Files          51       54       +3     
  Lines        4687     4688       +1     
  Branches     1022     1012      -10     
==========================================
- Hits         4046     3987      -59     
- Misses        456      522      +66     
+ Partials      185      179       -6     
Impacted Files Coverage Δ
signac/common/deprecation/__init__.py 0.00% <0.00%> (ø)
signac/common/errors.py 100.00% <ø> (ø)
signac/contrib/__init__.py 100.00% <ø> (ø)
signac/contrib/errors.py 94.11% <ø> (ø)
signac/synced_collections/_caching.py 0.00% <0.00%> (ø)
..._collections/buffers/memory_buffered_collection.py 100.00% <ø> (ø)
...ignac/synced_collections/data_types/synced_list.py 85.98% <ø> (ø)
signac/synced_collections/validators.py 91.48% <ø> (ø)
signac/testing.py 100.00% <ø> (ø)
signac/contrib/import_export.py 77.24% <71.42%> (-0.18%) ⬇️
... and 47 more

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

@AlainKadar AlainKadar closed this Aug 10, 2022
@AlainKadar AlainKadar force-pushed the feature/access-project-from-job branch from 1d63175 to 862fe97 Compare August 10, 2022 20:08
@AlainKadar AlainKadar deleted the feature/access-project-from-job branch August 10, 2022 20:11
@AlainKadar
Copy link
Contributor Author

Sorry, I completely messed up my local repo with changes from master, so moved this to a new PR: #808

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.

2 participants