Skip to content

bpo-37111: Added encoding keyword parameter to basicConfig.#13932

Closed
vsajip wants to merge 12 commits into
python:masterfrom
vsajip:fix-37111
Closed

bpo-37111: Added encoding keyword parameter to basicConfig.#13932
vsajip wants to merge 12 commits into
python:masterfrom
vsajip:fix-37111

Conversation

@vsajip

@vsajip vsajip commented Jun 9, 2019

Copy link
Copy Markdown
Member

@vsajip
vsajip requested a review from zooba June 9, 2019 17:16
@vsajip

vsajip commented Jun 9, 2019

Copy link
Copy Markdown
Member Author

@zooba Please review just to ensure I haven't missed anything ... thanks.

@zooba

zooba commented Jun 10, 2019

Copy link
Copy Markdown
Member

@vsajip We normally add encoding and errors parameters as pairs (at least, this is what I was asked to do when adding them to subprocess)

@zooba

zooba commented Jun 10, 2019

Copy link
Copy Markdown
Member

To be clear, I mean you should add an errors parameter as well. But the change overall looks good 👍

@vsajip
vsajip requested a review from rhettinger as a code owner June 10, 2019 18:44
@vsajip

vsajip commented Jun 10, 2019

Copy link
Copy Markdown
Member Author

Whoops, I made a mistake by doing a rebase - should I just kill this PR and start again?

@zooba

zooba commented Jun 11, 2019

Copy link
Copy Markdown
Member

@vsajip Yeah, looks like you undid some other changes, so probably best to restart.

FWIW, once you submit a PR and people start looking it at, you're best to just do regular merges and we'll squash it all at the end. That way you don't break the review workflow.

@vsajip

vsajip commented Jun 12, 2019

Copy link
Copy Markdown
Member Author

That's what I normally do, I just had a brain-fade this time :-(

@vsajip vsajip closed this Jun 12, 2019
@vsajip
vsajip deleted the fix-37111 branch June 12, 2019 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants