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.

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

Remove work_dir argument from ProviderService methods #606

Open
mattculler opened this issue Jan 15, 2025 · 2 comments
Open

Remove work_dir argument from ProviderService methods #606

mattculler opened this issue Jan 15, 2025 · 2 comments
Labels
Breaking Change Bug Something isn't working triaged

Comments

@mattculler
Copy link
Contributor

mattculler commented Jan 15, 2025

Bug Description

The ProviderService class's __init__ takes a work_dir argument, stored as self._work_dir. Some ProviderService methods use this, but others take their own work_dir and use that instead. This situation is confusing at best, a source of potential bugs at worst.

Ideally, all work_dir arguments to methods can be removed and replaced with self._work_dir. If some method(s) actually do need a different work dir, perhaps the argument should be named something different? Or if it truly should be called work_dir and methods need to take their own, comment(s) illuminating the situation should be added.

This same situation doesn't exist on other service classes.

To Reproduce

vi craft_application/services/provider.py

@mattculler mattculler added Bug Something isn't working good first issue Good for newcomers triaged labels Jan 15, 2025
Copy link

Thank you for reporting your feedback to us!

The internal ticket has been created: https://warthogs.atlassian.net/browse/CRAFT-3899.

This message was autogenerated

@canonical canonical deleted a comment from syncronize-issues-to-jira bot Jan 15, 2025
@lengau
Copy link
Contributor

lengau commented Feb 14, 2025

Could not reproduce.

(craft-application) lengau@ratel:~/Work/Code/craft-application$ vi craft_application/services/provider.py
Could not find command-not-found database. Run 'sudo apt update' to populate it.
vi: command not found

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Bug Something isn't working triaged
Projects
None yet
Development

No branches or pull requests

2 participants