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

Disallow print statements for debugging #1746

Merged
merged 2 commits into from
Jul 29, 2021
Merged

Conversation

MarcoGorelli
Copy link
Contributor

Description

Checklist

  • Follows official PR format
  • Includes a sample plot to visually illustrate the changes (only for plot-related functions)
  • New features are properly documented (with an example if appropriate)?
  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

Following up on @canyon289 's suggestion - this is to avoid problems like #1560

@canyon289
Copy link
Member

canyon289 commented Jul 26, 2021

Thanks for this contribution Marco.

One question related I think we should probably disable prints entirely and just use logging. A little outside of the scope of PR but thoughts?

For this PR scope I think this looks good

@OriolAbril
Copy link
Member

One question related I think we should probably disable prints entirely and just use logging. A little outside of the scope of PR but thoughts?

I think we already use only logging and warnings for all our messages. IIUC, the two modified lines that add file=sys.stdout are the only print occurrences in the codebase, and they are two tests.

@ahartikainen
Copy link
Contributor

Is this PR adding a check that there are no print statements in the codebase?

Why do we need to edit tests? They are hidden by default and when test fails, the printed parts are shown (also -s should show the prints when running locally)

Can we skip checking test folder?

@MarcoGorelli
Copy link
Contributor Author

Sure, done (bit unwieldy without pre-commit, but -not -path works)


(did I really accidentally open a PR from my main branch? oops 🤦 )

@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #1746 (a348c94) into main (ae83908) will decrease coverage by 4.04%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##             main    #1746      +/-   ##
==========================================
- Coverage   90.87%   86.82%   -4.05%     
==========================================
  Files         109      109              
  Lines       11844    11844              
==========================================
- Hits        10763    10284     -479     
- Misses       1081     1560     +479     
Impacted Files Coverage Δ
arviz/stats/stats.py 96.60% <ø> (ø)
arviz/data/inference_data.py 83.71% <100.00%> (ø)
arviz/data/io_pyro.py 96.24% <100.00%> (ø)
arviz/plots/backends/bokeh/loopitplot.py 95.29% <100.00%> (ø)
arviz/plots/backends/matplotlib/loopitplot.py 86.36% <100.00%> (ø)
arviz/data/io_pymc3_3x.py 18.18% <0.00%> (-72.06%) ⬇️
arviz/data/io_pystan.py 62.12% <0.00%> (-33.96%) ⬇️
arviz/data/io_cmdstanpy.py 70.68% <0.00%> (-24.76%) ⬇️
arviz/data/io_emcee.py 93.38% <0.00%> (-4.96%) ⬇️
arviz/utils.py 88.81% <0.00%> (-0.33%) ⬇️

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 ae83908...090a19c. Read the comment docs.

@canyon289 canyon289 merged commit 6de30a6 into arviz-devs:main Jul 29, 2021
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