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

Print statement to function #468

Closed
wants to merge 6 commits into from

Conversation

@tpltnt
Copy link
Contributor

tpltnt commented Mar 1, 2018

Hi there,

i changed all print statements to functions in order to be able to move Tahoe-LAFS to Python 3. In order to review this PR efficiently I propose the following.

  1. Everyone who wants to review looks at a file (currently named in the commit message).
  2. If a file looks good leave a message like "LGTM filename".
    Over time I will squash all reviewed commits into one stating "print statement to function". This way only the files which need a pair of eyes will be visible (implicitly via the commit messages). The final merge is going to be one big commit to ease conflict resolution and reduce littering the history.

Cheers,
tpltnt


This change is Reviewable

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 1, 2018

Codecov Report

Merging #468 into master will increase coverage by 1.71%.
The diff coverage is 81.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #468      +/-   ##
==========================================
+ Coverage   88.54%   90.26%   +1.71%     
==========================================
  Files         152      144       -8     
  Lines       28035    27588     -447     
  Branches     4009     3936      -73     
==========================================
+ Hits        24823    24901      +78     
+ Misses       2489     1948     -541     
- Partials      723      739      +16
Impacted Files Coverage Δ
src/allmydata/web/unlinked.py 84% <100%> (+0.12%) ⬆️
src/allmydata/util/iputil.py 65% <100%> (-3.07%) ⬇️
src/allmydata/node.py 93.24% <100%> (+0.01%) ⬆️
src/allmydata/immutable/encode.py 95.95% <100%> (ø) ⬆️
src/allmydata/web/filenode.py 93.95% <100%> (-0.29%) ⬇️
src/allmydata/util/rrefutil.py 70% <100%> (+1.03%) ⬆️
src/allmydata/mutable/publish.py 96.67% <100%> (+0.01%) ⬆️
src/allmydata/introducer/common.py 96.42% <100%> (+0.06%) ⬆️
src/allmydata/util/dictutil.py 100% <100%> (ø) ⬆️
src/allmydata/scripts/common.py 94.77% <100%> (+0.03%) ⬆️
... and 152 more

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 8142039...f03a5c3. Read the comment docs.

@meejah

This comment has been minimized.

Copy link
Contributor

meejah commented Mar 1, 2018

LGTM: src/allmydata/client.py src/allmydata/frontends/magic_folder.py src/allmydata/immutable/happiness_upload.py src/allmydata/scripts/magic_folder_cli.py

@meejah

This comment has been minimized.

Copy link
Contributor

meejah commented Mar 1, 2018

BTW, for the record we shouldn't care about coverage for this patch because any uncovered code was previously uncovered as well :/

@tpltnt tpltnt force-pushed the tpltnt:print-statement-to-function branch from d5baa65 to b178a28 Mar 2, 2018
@tpltnt tpltnt force-pushed the tpltnt:print-statement-to-function branch from b178a28 to d3f88e8 Mar 14, 2018
@tpltnt

This comment has been minimized.

Copy link
Contributor Author

tpltnt commented Mar 14, 2018

rebased with master and conflicts resolved

@tpltnt tpltnt mentioned this pull request Mar 16, 2018
@warner

This comment has been minimized.

Copy link
Member

warner commented Mar 20, 2018

looks good.. could you also (some time before landing) add from __future__ import print_function at the top of each file? I see it in some of them now, but not all. I'd bet that's going to save us in the future when some old PRs or forgetful developers add a non-parenthesized print statement or two.

@tpltnt

This comment has been minimized.

Copy link
Contributor Author

tpltnt commented Mar 20, 2018

rebased with master and added from __future__ import print_function in each file missing it.

@tpltnt tpltnt force-pushed the tpltnt:print-statement-to-function branch 5 times, most recently from 09e3d26 to 0ed4288 Mar 20, 2018
@tpltnt tpltnt force-pushed the tpltnt:print-statement-to-function branch 3 times, most recently from 883bae5 to e9c31a5 Mar 30, 2018
@tpltnt tpltnt force-pushed the tpltnt:print-statement-to-function branch from e9c31a5 to efaa29e May 1, 2018
@tpltnt tpltnt force-pushed the tpltnt:print-statement-to-function branch from efaa29e to 19814b1 May 17, 2018
@tpltnt tpltnt force-pushed the tpltnt:print-statement-to-function branch from 19814b1 to f03a5c3 Jul 3, 2018
@exarkun

This comment has been minimized.

Copy link
Member

exarkun commented Mar 25, 2019

Similar effort is progressing separately from this. Thanks for your efforts @tpltnt sorry this didn't come together.

@exarkun exarkun closed this Mar 25, 2019
@tpltnt tpltnt deleted the tpltnt:print-statement-to-function branch Apr 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.