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.index #591

Merged
merged 4 commits into from
Aug 10, 2021
Merged

deprecate Project.index #591

merged 4 commits into from
Aug 10, 2021

Conversation

atravitz
Copy link
Collaborator

@atravitz atravitz commented Aug 4, 2021

addresses issue #588

Description

Motivation and Context

Indexing is deprecated and we'd like to remove indexing.py in v 2.0, but to do so we need to remove dependencies in project.py.

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.

@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #591 (3023c03) into master (70d480e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 3023c03 differs from pull request most recent head e50af81. Consider uploading reports for the commit e50af81 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #591   +/-   ##
=======================================
  Coverage   78.27%   78.27%           
=======================================
  Files          65       65           
  Lines        7069     7070    +1     
  Branches     1310     1310           
=======================================
+ Hits         5533     5534    +1     
  Misses       1231     1231           
  Partials      305      305           
Impacted Files Coverage Δ
signac/contrib/project.py 85.30% <100.00%> (+0.01%) ⬆️

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 70d480e...e50af81. Read the comment docs.

@bdice bdice mentioned this pull request Aug 6, 2021
12 tasks
@atravitz atravitz marked this pull request as ready for review August 9, 2021 15:33
@atravitz atravitz requested review from a team as code owners August 9, 2021 15:33
@atravitz atravitz requested review from cbkerr, tommy-waltmann and bdice and removed request for a team August 9, 2021 15:33
@atravitz
Copy link
Collaborator Author

atravitz commented Aug 9, 2021

Looks like @bdice handled dependencies etc. in #593, so I'm just adding the deprecation message here.

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.

thanks!

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.

I added a changelog line. Merging. We will still need to go back and add deprecations to individual functions accepting an index argument. See #588 for a list of those methods.

@bdice bdice enabled auto-merge (squash) August 10, 2021 02:29
@bdice bdice merged commit 8549adf into master Aug 10, 2021
@bdice bdice deleted the deprecate_indexing branch August 10, 2021 02:51
@atravitz
Copy link
Collaborator Author

I added a changelog line. Merging. We will still need to go back and add deprecations to individual functions accepting an index argument. See #588 for a list of those methods.

@bdice I'm doing this now. Is there an easy way to only show a deprecation warning when a user passes the index argument? Otherwise I'll put it on all of the functions that are not already deprecated.

@bdice
Copy link
Member

bdice commented Aug 11, 2021

@atravitz We don't want to deprecate the whole method. Try something like this:

warnings.warn(DOC_FILTER_WARNING, DeprecationWarning)

For example:

# At the top of the `project.py` file:
INDEX_DEPRECATION_WARNING = "The index argument is deprecated and will be removed in signac 2.0."

# In methods with an `index` argument, put this near the top of the method:
if index is not None:
    warnings.warn(INDEX_DEPRECATION_WARNING, DeprecationWarning)

@b-butler b-butler added this to the v1.8.0 milestone Sep 19, 2022
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