Skip to content

Conversation

@lrandersson
Copy link
Contributor

Description

Needs to be rebased once #1133 is merged.

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@github-project-automation github-project-automation bot moved this to 🆕 New in 🔎 Review Dec 17, 2025
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Dec 17, 2025
return {"text": "TODO"}


class UninstallBat:
Copy link
Contributor Author

@lrandersson lrandersson Dec 17, 2025

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.

@github-project-automation github-project-automation bot moved this from 🆕 New to 🏁 Done in 🔎 Review Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed [bot] added once the contributor has signed the CLA

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants