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

Deprecate project id #644

Merged
merged 11 commits into from
Feb 23, 2022
Merged

Deprecate project id #644

merged 11 commits into from
Feb 23, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Nov 9, 2021

Description

Projects now have a default name, and setting any custom name triggers a warning.

Motivation and Context

Related to #541. This change provides a softer off-ramp for current users with scripts where they provide the name parameter for project initialization. Rather than removing it entirely, this PR provides a warning that this parameter will be removed in 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.

@vyasr vyasr added this to the v1.8.0 milestone Nov 9, 2021
@vyasr vyasr self-assigned this Nov 9, 2021
@vyasr vyasr requested review from a team as code owners November 9, 2021 06:37
@vyasr vyasr requested review from cbkerr and Tobias-Dwyer and removed request for a team November 9, 2021 06:37
@codecov
Copy link

codecov bot commented Nov 9, 2021

Codecov Report

Merging #644 (b5ac8b0) into master (bb5c07d) will decrease coverage by 0.01%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #644      +/-   ##
==========================================
- Coverage   78.56%   78.54%   -0.02%     
==========================================
  Files          65       65              
  Lines        7137     7141       +4     
  Branches     1564     1565       +1     
==========================================
+ Hits         5607     5609       +2     
- Misses       1227     1228       +1     
- Partials      303      304       +1     
Impacted Files Coverage Δ
signac/diff.py 100.00% <ø> (ø)
signac/contrib/project.py 85.03% <66.66%> (-0.18%) ⬇️
signac/__main__.py 71.89% <100.00%> (ø)

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 bb5c07d...b5ac8b0. Read the comment docs.

@b-butler
Copy link
Member

@cbkerr @Tobias-Dwyer will you have time to review this PR?

Copy link
Member

@cbkerr cbkerr left a comment

Choose a reason for hiding this comment

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

Do we also have to update the docstring of init_project on line 3045?

signac/contrib/project.py Outdated Show resolved Hide resolved
signac/contrib/project.py Outdated Show resolved Hide resolved
@vyasr vyasr requested a review from cbkerr November 19, 2021 20:43
Copy link
Member

@cbkerr cbkerr left a comment

Choose a reason for hiding this comment

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

Check that my changes to init_project on lines 3046 and 3055 are okay.

Otherwise it looks good!

@vyasr
Copy link
Contributor Author

vyasr commented Dec 1, 2021

Check that my changes to init_project on lines 3046 and 3055 are okay.

Otherwise it looks good!

Thanks @cbkerr! Your change looks right to me. I'm going to hold off merging this until #643 is resolved just in case we run into any blockers with the deprecation there.

@vyasr vyasr mentioned this pull request Dec 1, 2021
18 tasks
signac/contrib/project.py Outdated Show resolved Hide resolved
signac/contrib/project.py Outdated Show resolved Hide resolved
signac/contrib/project.py Outdated Show resolved Hide resolved
changelog.txt Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented Jan 8, 2022

Still waiting on #643.

@vyasr vyasr requested a review from bdice February 21, 2022 20:06
@vyasr
Copy link
Contributor Author

vyasr commented Feb 21, 2022

The relevant changes from #643 were separated into #677, which has now been merged onto next, so we should be good to go on this PR if you approve @bdice.

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.

LGTM!

@vyasr vyasr merged commit c291e1e into master Feb 23, 2022
@vyasr vyasr deleted the refactor/deprecate_project_id branch February 23, 2022 17: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.

4 participants