-
Notifications
You must be signed in to change notification settings - Fork 0
Briefcase uninstall #2
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
Conversation
| return {"text": "TODO"} | ||
|
|
||
|
|
||
| class UninstallBat: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach is a bit different from how we currently handle run_installation.bat. I'd like to hear some feedback/thoughts on what you (the reviewer) thinks.
I went with this class-based approach because I think it's better in terms of future maintenance and also makes unit testing very convenient, and easy to read. All the code for it is in one place and easy to follow. I think most of constructor is written in a "scripted" approach and I think that works for small projects, but for larger projects it can quickly become cumbersome. I would argue the project could benefit from a more object-oriented approach in general.
Moreover, simplifying the testing would be a good win for the project. One of the major pain points in constructor is the difficulty of running the tests locally, and to do it fast and "as close to production" as possible.
If we don't wanna go with this approach, it wouldn't take a lot of effort to simply add a pre_uninstall.bat and change the implementation, but I would propose we add something similar also for run_installation.bat (I'm happy to do that update if we go with it).
|
Closed due to conda#1143 |
Description
conda#1102
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?