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

Skip adding a duplicated consumer to pip's logger #1723

Merged
merged 4 commits into from Apr 25, 2014

Conversation

Projects
None yet
5 participants
@Ivoz
Member

Ivoz commented Apr 15, 2014

Fixes #1618

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Apr 15, 2014

Member

@pfmoore would be awesome if you'd be able to test this also works on windows, i.e you stop getting output like #1618 (comment) (number of log lines in idempotent)

Member

Ivoz commented Apr 15, 2014

@pfmoore would be awesome if you'd be able to test this also works on windows, i.e you stop getting output like #1618 (comment) (number of log lines in idempotent)

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Apr 15, 2014

Member

@Ivoz I'll try to get round to it, but probably not till after Easter - it's crazy busy round here at the moment :-( If I haven't responded by next week, ping me about it.

Member

pfmoore commented Apr 15, 2014

@Ivoz I'll try to get round to it, but probably not till after Easter - it's crazy busy round here at the moment :-( If I haven't responded by next week, ping me about it.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Apr 16, 2014

Contributor

lgtm, but we should add a test to tests.unit.test_log

Contributor

qwcode commented Apr 16, 2014

lgtm, but we should add a test to tests.unit.test_log

@piotr-dobrogost

This comment has been minimized.

Show comment
Hide comment
@piotr-dobrogost

piotr-dobrogost Apr 16, 2014

Sorry for nitpicking but wouldn't it be more natural to postpone doing anything with consumer unless it's going to be added? I mean to move this code inside the next if not consumer_exists:.

piotr-dobrogost commented on pip/log.py in da867b1 Apr 16, 2014

Sorry for nitpicking but wouldn't it be more natural to postpone doing anything with consumer unless it's going to be added? I mean to move this code inside the next if not consumer_exists:.

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Apr 16, 2014

Owner

Probably a micro-optimization, but its not harmful either :) See next commit

Owner

Ivoz replied Apr 16, 2014

Probably a micro-optimization, but its not harmful either :) See next commit

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Apr 16, 2014

Member

Tests added

Member

Ivoz commented Apr 16, 2014

Tests added

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Apr 23, 2014

Member

@qwcode these tests look good to you? Otherwise would like to merge this

Member

Ivoz commented Apr 23, 2014

@qwcode these tests look good to you? Otherwise would like to merge this

Show outdated Hide outdated tests/unit/test_log.py Outdated
@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Apr 23, 2014

Member

Works on Windows (including the colours remaining as expected). So looks good to me - sorry for the delay.

Member

pfmoore commented Apr 23, 2014

Works on Windows (including the colours remaining as expected). So looks good to me - sorry for the delay.

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Apr 23, 2014

Member

I swear I looked for ages at pytest.patch for how to patch stuff but couldn't figure it out... turns out you need monkeys to help you.

Howsat ^?

Member

Ivoz commented Apr 23, 2014

I swear I looked for ages at pytest.patch for how to patch stuff but couldn't figure it out... turns out you need monkeys to help you.

Howsat ^?

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Apr 23, 2014

Member

@pfmoore cheers for checking its all good on the Windows side ^_^ Also why I added the second test.

Member

Ivoz commented Apr 23, 2014

@pfmoore cheers for checking its all good on the Windows side ^_^ Also why I added the second test.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Apr 23, 2014

Contributor

@Ivoz, tests look ok to me.

Contributor

qwcode commented Apr 23, 2014

@Ivoz, tests look ok to me.

@Ivoz Ivoz merged commit 11e928a into pypa:develop Apr 25, 2014

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Apr 25, 2014

Member

Cheers @qwcode 😁

Member

Ivoz commented Apr 25, 2014

Cheers @qwcode 😁

@Ivoz Ivoz deleted the Ivoz:duplicate_consumers branch Jun 11, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment