Skip to content

Conversation

@KillingTree
Copy link

console scope error case while using pipx:

  File "/XXXX/.local/share/pipx/venvs/pywhisker/lib/python3.11/site-packages/pywhisker/pywhisker.py", line 733, in error
    console.print("{}[!]{} {}".format("[bold red]", "[/bold red]", message), highlight=False)
    ^^^^^^^
NameError: name 'console' is not defined. Did you mean: 'Console'?

Since console is only used within Logger class, I moved the declaration from if __name__ == '__main__': condition statement block to Logger constructor making it a property.

logger scope error case while using pipx or calling pywhisker as a script:

 File "/XXXXXX/.local/share/pipx/venvs/pywhisker/lib/python3.11/site-packages/pywhisker/pywhisker.py", line 258, in __init__
    logger.debug('Initializing domainDumper()')
    ^^^^^^
NameError: name 'logger' is not defined. Did you mean: 'Logger'?

logger definition was moved to main function while doing the pipx support but can't be found within ShadowCredentials execution.
ShadowCredentials class assume a global logger so instead of making it global in main, I give the main instance to ShadowCredentials so he can refer to its copy or create it by itself if it's not defined within constructor.

It's still an implicit coupling but not as bad design than a global variable and it was much easier to do this as the state of the code.
If I had more time, isolating Logger and ShadowCredentials would be my choice and declare the Logger using dependency injection

I tested this code as a pipx module and a standalone script against a HTB prolab, no more errors on my side.

Feel free to ping me if I need to change a few things ! :)

@ShutdownRepo
Copy link
Owner

do you mind fixing the conflict @KillingTree ?

@KillingTree
Copy link
Author

Done, I kept changes from the other PR and modified logger calls accordingly to the new scope !

@ShutdownRepo
Copy link
Owner

Should be fixed thanks to #24
Please feel free to reopen if things don't work as they should 🙏
(Thank you @KillingTree for the help!!)

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.

2 participants