-
Notifications
You must be signed in to change notification settings - Fork 176
Briefcase uninstall #1143
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
Briefcase uninstall #1143
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).
This can also be expanded upon at a later point, for instance by changing the lists of commands to jinja-based templating.
Description
Needs to be rebased once #1133 is merged.
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?