bpo-39206: Add encoding detecting to modulefinder

[LucianaMarques]

Fixing issue 39206.

https://bugs.python.org/issue39206

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@LucianaMarques

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

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

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

[LucianaMarques]

Just recording, I just signed the CLA.

[LucianaMarques]

@asottile should I add a file to Misc/NEWSs.d? I did't find much informaiton on the dev guide.

[asottile]

@asottile should I add a file to Misc/NEWSs.d? I did't find much informaiton on the dev guide.

yep! there's two ways to do that -- iirc clicking on the details link will take you to the web UI for it, otherwise you can pip install blurb locally and then run blurb add

[plokmijnuhby]

Could you also add some more tests for this issue?

[LucianaMarques]

yep! there's two ways to do that -- iirc clicking on the details link will take you to the web UI for it, otherwise you can pip install blurb locally and then run blurb add

Thanks, will do it!

Could you also add some more tests for this issue?

Of course! I will add some tests to ModuleFinder. I also thought about guaranteeing this in a broader context whenever there is an open() to a file in a future PR. Is there any specific test case you would like me to include? Thank you so much for the review!

[plokmijnuhby]

It might be at least worth setting up tests to open files with odd encodings. Test it on a file with some Unicode characters in it, and on files with PEP263 style encoding declarations.

On a different note, I would point out that when running files, they are opened in bytes mode and that's it. Encoding is inferred during the compilation process, simply by running the compile() function on the bytestring read from the file, with no need to import tokenize or open in 'r' mode or anything else.

[LucianaMarques]

@plokmijnuhby I've added one example, could you please give me feedback on it? If it's ok, I will add one ow two more with different encoding types or testing default is type to UTF-8. Thanks!

[LucianaMarques]

Hey guys, just to say that I have been away for a few days but I'll come back to this tomorrow. Sorry for my absence!

[asottile]

otherwise looks great -- thanks for sticking with this!

[LucianaMarques]

@asottile all checks pass now, do I need to add anything else? Also, I don't know why my PR is still as CLA not signed, but I've already signed it and also registered at python.org.

[asottile]

the CLA thing is weird, the app says it isn't linked to your github user -- did you maybe sign it with a different email / account?

[LucianaMarques]

the CLA thing is weird, the app says it isn't linked to your github user -- did you maybe sign it with a different email / account?

I certainly did with the same e-mail, I'll check if I can link these things!

[LucianaMarques]

@asottile I'm all set now! What's the next step? Thanks for your support!

[asottile]

now we wait! this is the unfortunate part about committing to cpython -- there are very few core devs which can merge a PR

this looks good though :)

[LucianaMarques]

now we wait! this is the unfortunate part about committing to cpython -- there are very few core devs which can merge a PR

this looks good though :)

@asottile thanks! should I squash the changes into one commit while we wait?

[asottile]

iirc it doesn't matter, commits will get squashed upon merge

[LucianaMarques]

Just registering here that I accidentally added a commit with blurp_it when trying to add a commit with it in my other current open PR #18045. I have rebased it out now.

[serhiy-storchaka]

Seems similar changes was already made in #19488.