Skip to content

Conversation

@wmuth
Copy link
Contributor

@wmuth wmuth commented Mar 25, 2025

TLDR: This PR addresses issue #927

  • make cheat implicitly relied on install since it just called mi, made the dependency explicit
  • created misc/test-utils.sh for containing extra logic to not bloat up misc/test. The file contains logic for checking which mi version the user requested and making sure it exists.
  • misc/test sources test-utils.sh and calls the function inside of it.

Copy link
Contributor

@elegios elegios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice that someone is looking at these scripts with an outside perspective. I'd be a bit cautious of automatically and implicitly doing things that aren't the main task of each script. Instead, I'd prefer to either give an informative error message or an explicit confirmation dialog.

@wmuth
Copy link
Contributor Author

wmuth commented Mar 26, 2025

@elegios

I could put all the installs behind y/N prompts. Of course it could also just print error messages but I personally much preferred the script having the ability to also perform the needed change, but I see your point about asking the user before doing anything. EDIT: Should I add a --noninteractive flag potentially which skips the y/N prompt and assumes N? Potentially github actions could use this as well though it shouldn't have issues as it bootstraps mi before running the test script so the file exists check is fine.

For the Tup part, potentially I could move the checks to happen after the check wether to use tup or make as the test-runner? In that case the command/file exists check would only be run if we are using make and can't use tup's versions of the binaries. And of coure it can ask the user before actually running the make targets.

@elegios
Copy link
Contributor

elegios commented Mar 27, 2025

I think asking before running another make target makes sense, as well as taking tup or not into account. A non-interactive flag would be nice, but not super-necessary I think, depends on how much it affects the complexity of the script(s), so I think that's up to you. I agree that it shouldn't matter for our automated uses.

@wmuth
Copy link
Contributor Author

wmuth commented Mar 27, 2025

Merging this PR closes #927 .

@wmuth wmuth requested a review from elegios March 27, 2025 18:03
@david-broman david-broman merged commit b174ba6 into miking-lang:develop Mar 30, 2025
1 of 2 checks passed
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.

3 participants