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.

Sign up for GitHub

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

Jump to bottom

Add logging namespace for all relevant aexpect modules #89

Merged
merged 1 commit into from
Nov 19, 2021
Merged

Add logging namespace for all relevant aexpect modules #89

merged 1 commit into from
Nov 19, 2021

Conversation

pevogam
Copy link
Contributor

@pevogam pevogam commented Nov 11, 2021

As Aexpect is a library used by other applications, it makes sense
to limits its logging and not log directly to the root logger.

Signed-off-by: Plamen Dimitrov [email protected]

@pevogam
Copy link
Contributor Author

pevogam commented Nov 11, 2021

@beraldoleal This PR is allowing us to restore Aexpect's logging in Avocado, any review in your spare cycles is appreciated. I haven't tested it yet and will go about that right now.

@pevogam
Copy link
Contributor Author

pevogam commented Nov 11, 2021 •
edited
Loading

Noting down here, it might be better to just go all the way to using log on each message in order to avoid a collision where Avocado VT might do from aexpect.remote import * (from virttest/remote.py) and end up reimporting the logging module. Of course, we can also provide a public API through __all__ module attribute and importing wildcards are a bad idea to begin with.

willianrampazzo
willianrampazzo reviewed Nov 11, 2021
View reviewed changes
aexpect/client.py Outdated Show resolved Hide resolved
@pevogam pevogam force-pushed the logging-namespace branch 2 times, most recently from 786b065 to 6a4b3aa Compare November 19, 2021 10:58
@pevogam
Copy link
Contributor Author

pevogam commented Nov 19, 2021

Since there aren't more opinions from the last weeks and the project size is fairly small I took the liberty to bring this PR to a more finalized state and did the replacement not just for imports but also for all emitted logging messages. Any further reviews that could bring this PR to fruition are welcome.

beraldoleal
beraldoleal reviewed Nov 19, 2021
View reviewed changes
Copy link
Member

@beraldoleal beraldoleal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @pevogam this LGTM, just one small comment.

aexpect/client.py Outdated Show resolved Hide resolved
@pevogam
Add logging namespace for all relevant aexpect modules
170291d 
As Aexpect is a library used by other applications, it makes sense
to limits its logging and not log directly to the root logger.

Signed-off-by: Plamen Dimitrov <[email protected]>
@beraldoleal beraldoleal merged commit d216b6d into avocado-framework:master Nov 19, 2021
@pevogam pevogam deleted the logging-namespace branch November 22, 2021 11:46
@luckyh luckyh mentioned this pull request Mar 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willianrampazzo willianrampazzo willianrampazzo approved these changes

@beraldoleal beraldoleal beraldoleal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pevogam @beraldoleal @willianrampazzo