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

kwentine
Copy link
Contributor

@kwentine kwentine 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
Copy link

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
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.

@kwentine
Copy link
Contributor Author

kwentine 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
Copy link
Contributor

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.

@kwentine
Copy link
Contributor Author

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
Copy link
Member

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
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.

@kwentine
Copy link
Contributor Author

😊 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
Copy link
Contributor

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

@miss-islington miss-islington merged commit 9e14e49 into python:master Oct 26, 2018
@brettcannon
Copy link
Member

@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. 😄

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.

6 participants