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

bpo-35024: Remove redundant and possibly incorrect verbose message after writing '.pyc' #9998

Merged
merged 5 commits into from Oct 26, 2018

Conversation

@qagren
Copy link
Contributor

qagren commented Oct 20, 2018

Since SourceFileLoader.set_data() catches exceptions raised by _write_atomic() and logs an informative message consequently, always logging successful outcome in 'SourceLoader.get_code()' seems redundant.

https://bugs.python.org/issue35024

@the-knights-who-say-ni

This comment has been minimized.

Copy link

the-knights-who-say-ni commented Oct 20, 2018

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@ncoghlan

This comment has been minimized.

Copy link
Contributor

ncoghlan commented Oct 25, 2018

The commit needs a "make regen-importlib" before the tests will run.
While it's a relatively minor change, I think it's worth adding a NEWS entry for, just in case someone is in the habit of trawling the debug messages to find out which pyc files are getting written.

@qagren

This comment has been minimized.

Copy link
Contributor Author

qagren commented Oct 25, 2018

I've just learnt about make regen-importlib for another minor fix today, will do sorry.

Another question (please let me know if it's not the place to seek for such advice I don't want to waste your time): do I rebase this branch on top of master and do a push --force (to avoid merge conflicts with recently regenerated importlib_external.h) ? Or will it be taken care of "behind the scenes"?

@ncoghlan

This comment has been minimized.

Copy link
Contributor

ncoghlan commented Oct 25, 2018

For review purposes, merging master into the PR branch is better (since GitHub does a better job of figuring out the appropriate diff with earlier versions of the review PR).

There's then a final squash & merge at commit time that avoids complicating the development history with those additional merges.

@qagren

This comment has been minimized.

Copy link
Contributor Author

qagren commented Oct 25, 2018

Thanks. Merged master, ran make regen-importlib and added news entry (although I was not really sure about how much details to put in the latter)

@brettcannon

This comment has been minimized.

Copy link
Member

brettcannon commented Oct 26, 2018

Just for reference, the main logging call is handled by:

try:
_write_atomic(path, data, _mode)
_bootstrap._verbose_message('created {!r}', path)
except OSError as exc:
# Same as above: just don't write the bytecode.
_bootstrap._verbose_message('could not create {!r}: {!r}', path,
exc)

I've left comments on the issue to discuss a potential alternative solution.

@brettcannon

This comment has been minimized.

Copy link
Member

brettcannon commented Oct 26, 2018

Actually, my alternative is unnecessarily complex. 😄 I tweaked the news entry, so once CI goes green this will be merged.

@qagren

This comment has been minimized.

Copy link
Contributor Author

qagren commented Oct 26, 2018

😊 proud of my first non-typo-fixing contribution. Next step: try to add a line of code 😅
Thanks for the help and mention in NEWS.d!

@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Oct 26, 2018

@qagren: Status check is done, and it's a success .

@miss-islington miss-islington merged commit 9e14e49 into python:master Oct 26, 2018
5 checks passed
5 checks passed
Azure Pipelines PR #20181026.59 succeeded
Details
bedevere/issue-number Issue number 35024 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@brettcannon

This comment has been minimized.

Copy link
Member

brettcannon commented Oct 26, 2018

@qagren honestly, code removal is more important than adding. Less code (typically) means less maintenance. When we started Python 3.0 we had an informal competition over who could remove the most amount of code from the stdlib. 😄

yahya-abou-imran added a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018
…ter writing '.pyc' (pythonGH-9998)

Since `SourceFileLoader.set_data()` catches exceptions raised by `_write_atomic()` and logs an informative message consequently, always logging successful outcome in 'SourceLoader.get_code()' seems redundant.  



https://bugs.python.org/issue35024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.