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

ExampleTestSuite: Also support mill command in addition to ./mill #2865

Merged
merged 9 commits into from
Nov 8, 2023

Conversation

lefou
Copy link
Member

@lefou lefou commented Nov 8, 2023

Replace ./mill with mill in most examples, as this is easier to read. I kept the ./mill in the first simple scala project, which also describes that there is a ./mill script in each example archive.

@lefou lefou changed the title example test suite ExampleTestSuite: Also support mill command in addition to ./mill Nov 8, 2023
@lihaoyi
Copy link
Member

lihaoyi commented Nov 8, 2023

Should we support both? I feel like supporting 1 should be enough, and would be good to enforce some degree of consistency throughout the docsite which gets built from these test suites. If we want to use mill, we should cut over everything to mill

@lihaoyi
Copy link
Member

lihaoyi commented Nov 8, 2023

I wanted to go with ./mill to remove the need for installation from the example projects, as all the zip downloads come with a ./mill script built in, so in theory anyone just needs a JVM installed and they can download the zip and follow the instructions and be ready to go

@lefou
Copy link
Member Author

lefou commented Nov 8, 2023

Yeah, I think I got your initial motivation for ./mill. Yet, I think it reads better with just mill. It also encourages to just install mill on the system and avoid dependency on some local script. It gives a newcomer a bit less hack-ish feeling. I for myself always prefer a system-installed tool over some repo-local script.

I think keeping both variants supported is not really adding inconsistency. As we explain the user that we have that script in example 1-simple-scala, I thought we should keep the ./mill at least in this example, and it will still work as an executable test.

@lihaoyi
Copy link
Member

lihaoyi commented Nov 8, 2023

sounds good to me, go for it

@lefou lefou marked this pull request as ready for review November 8, 2023 13:17
Base automatically changed from doc-example-rendering to main November 8, 2023 13:17
@lefou lefou merged commit c12d024 into main Nov 8, 2023
37 checks passed
@lefou lefou deleted the example-test-suite branch November 8, 2023 15:56
@lefou lefou added this to the 0.11.6 milestone Nov 8, 2023
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.

None yet

2 participants