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

Deprecation of create_access_module method #308

Merged
merged 20 commits into from
Apr 3, 2020

Conversation

kidrahahjo
Copy link
Collaborator

@kidrahahjo kidrahahjo commented Mar 19, 2020

Description

I have deprecated the create_access_module method but haven't adapted with its deprecation warning yet as no warnings were showing up on my local machine.

Motivation and Context

Fixes #303, fixes #317.

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.

Example for a changelog entry: Fix issue with launching rockets to the moon (#101, #212).

@kidrahahjo kidrahahjo requested review from a team as code owners March 19, 2020 16:24
@bdice bdice requested review from a team, tommy-waltmann and zhou-pj and removed request for a team March 19, 2020 21:57
@bdice bdice added the enhancement New feature or request label Mar 19, 2020
@bdice bdice added this to the v1.5.0 milestone Mar 19, 2020
signac/contrib/project.py Outdated Show resolved Hide resolved
@bdice
Copy link
Member

bdice commented Mar 19, 2020

@kidrahahjo Next steps:

  • Make sure the deprecation warning appears when calling signac project --access, in addition to the Python API project.create_access_module(). It doesn't look like it's showing in the test output right now, but I'm not sure if it's just untested or if it's not showing for another reason.
  • Hide this deprecation warning wherever it occurs in testing output, use pytest's "deprecated call" feature or something like that.
  • Add access modules to the "Deprecated" section of the changelog for the next release.

@kidrahahjo
Copy link
Collaborator Author

@kidrahahjo Next steps:

  • Make sure the deprecation warning appears when calling signac project --access, in addition to the Python API project.create_access_module(). It doesn't look like it's showing in the test output right now, but I'm not sure if it's just untested or if it's not showing for another reason.
  • Hide this deprecation warning wherever it occurs in testing output, use pytest's "deprecated call" feature or something like that.
  • Add access modules to the "Deprecated" section of the changelog for the next release.

@bdice Can I ignore the deprecation warning globally instead of using the pytest's "deprecated call" feature?

@codecov
Copy link

codecov bot commented Mar 20, 2020

Codecov Report

Merging #308 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   76.18%   76.19%   +0.01%     
==========================================
  Files          43       43              
  Lines        7075     7078       +3     
==========================================
+ Hits         5390     5393       +3     
  Misses       1685     1685              
Impacted Files Coverage Δ
signac/__main__.py 80.04% <100.00%> (+0.04%) ⬆️
signac/contrib/project.py 91.59% <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 fac9ccc...d910eb7. Read the comment docs.

Copy link
Contributor

@tommy-waltmann tommy-waltmann left a comment

Choose a reason for hiding this comment

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

LGTM

@bdice
Copy link
Member

bdice commented Mar 20, 2020

@kidrahahjo Ignoring the warning is not quite the same. Using pytest.deprecated_call ensures that the code actually triggers a deprecation warning. That means it is testable behavior.
https://docs.pytest.org/en/3.1.0/warnings.html#ensuring-function-triggers

@kidrahahjo
Copy link
Collaborator Author

@kidrahahjo Ignoring the warning is not quite the same. Using pytest.deprecated_call ensures that the code actually triggers a deprecation warning. That means it is testable behavior.
https://docs.pytest.org/en/3.1.0/warnings.html#ensuring-function-triggers

@bdice I will have a look and then do the changes where required. Thank you for providing the link.

@kidrahahjo
Copy link
Collaborator Author

@tommy-waltmann @zhou-pj
Even after testing the module test_shell.py without using pytest.deprecated_call(): then too it doesn't show up any warning. So the tests are failing. I don't know why the warning is now showing up. I think that this should not be merged until the deprecation warning starts showing up. Please help me with your views on this comment.

@tommy-waltmann
Copy link
Contributor

@kidrahahjo There is nothing that immediately jumps out to me as the reason a warning is not being generated, you may have to experiment a little on your own machine as to why this is happening.

For example, you might run only this unit test and make sure, via pdb or a print statement, that the code does in fact enter the if args.access block. You might also want to check and make sure that the print statement right after the warnings.warn line is being printed to the console.

If neither of those work, you can try some of your own experiments to determine what is going on.

@kidrahahjo
Copy link
Collaborator Author

@tommy-waltmann @zhou-pj @bdice
I don't think that we should use pytest.deprecated_call() in the tests because as we are not calling the function directly rather we're running python -m signac project --access in the tests through the call method. If a user runs this command then he might get the warning. Please let me know if I'm going in the wrong direction.

@bdice
Copy link
Member

bdice commented Mar 31, 2020

@kidrahahjo I am going to push some changes. I finally figured out something that had been confusing me. I forgot that the deprecation warnings won't actually appear until __version__ >= deprecated_in. So until we release 1.5.0, the deprecation warnings will not appear. I kept thinking I would see deprecations appearing in the output but they didn't show up...

@@ -115,6 +116,9 @@
"""


warnings.simplefilter("default")
Copy link
Member

Choose a reason for hiding this comment

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

@glotzerlab/signac-maintainers Can I get a second opinion on this one-line fix for issue #317? I put it in this PR because it's related to deprecation warnings.

Copy link
Member

Choose a reason for hiding this comment

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

I verified that warnings are sent to stderr, not stdout.

@kidrahahjo
Copy link
Collaborator Author

kidrahahjo commented Mar 31, 2020

@kidrahahjo I am going to push some changes. I finally figured out something that had been confusing me. I forgot that the deprecation warnings won't actually appear until __version__ >= deprecated_in. So until we release 1.5.0, the deprecation warnings will not appear. I kept thinking I would see deprecations appearing in the output but they didn't show up...

@bdice I tried with a code which had deprecated_in < __version__ but that too didn't get the warning.

@kidrahahjo
Copy link
Collaborator Author

I also tried using the print statement with file=sys.stdout. That too didn't work out for helping me debug. Can you let me know why that happened?

setup.cfg Outdated Show resolved Hide resolved
tests/test_shell.py Outdated Show resolved Hide resolved
@bdice bdice self-requested a review April 3, 2020 17:18
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 approve. Thanks @kidrahahjo! I think this is ready to merge once I get a second opinion from someone on warnings in the signac CLI.

@bdice bdice merged commit beefdf8 into glotzerlab:master Apr 3, 2020
@kidrahahjo kidrahahjo deleted the access_deprecation branch April 4, 2020 13:00
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
3 participants