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

Toolkit: MUnit, os-lib, sttp and upickle tutorials #7

Closed
wants to merge 181 commits into from
Closed

Conversation

adpi2
Copy link
Member

@adpi2 adpi2 commented Nov 14, 2022

[UPDATED]

image

sjrd and others added 5 commits November 14, 2022 14:53
- Use you instead of we
- Remove Scala-CLI tab on Scala code
- Add info about InvalidTestClassError
Tutorial for MUnit async tests.
@szymon-rd
Copy link
Collaborator

szymon-rd commented Nov 15, 2022

I think that this article is pretty long and has a complex structure. In my opinion we could add value & perceived simplicity if we tried to make it more concise. I think that we could stick with scala-cli in these examples (while giving some link on installing dependencies to sbt/mill). Then we can avoid having to explain the files structure, we just say that you create that file and it could already have the using directive on top. That way we also get rid of the banner on top. I think I would also get rid of the red banner with the notice about the InvalidTestClassError - I think it could be just a text, I don't perceive it as that important, more of a side-note that may help somebody recover when he creates an object instead. I believe that we also could remove the explanation of the failing test log, as it seems pretty readable on its own.
In general I believe that in these articles we should optimise for making them as short as possible while enabling the users to achieve the goal, and reduce the information noise. In this case this noise is IMO a bit introduced by banner on top, red banner, frames for selecting the build tool that contain these file trees and long output snippet.
As another option aside from the default scala-cli we could maybe have a short hyperlink on other options for installing the library. I think that the site that this link would lead to could even be some kind of a dynamic page that generates simple dependency installation instructions for multiple buildtools. It could even receive name of lib and the artifact data in the URL and generate the instructions.

@adpi2
Copy link
Member Author

adpi2 commented Nov 16, 2022

Thanks @szymon-rd for your feedback.

I think that we could stick with scala-cli in these examples (while giving some link on installing dependencies to sbt/mill).

While I agree we should encourage people to use Scala CLI and promote the "simplicity" of Scala CLI, I don't want the many users of sbt to feel that the Toolkit is not made for them. It is indeed easier to use the Toolkit with Scala CLI but it is just a set of libraries: it is not tied to any particular build tool.

I want to keep the tabs. They show clearly that we are trying not to divide the community. Also people can easily compare and see that the Scala CLI solution is simpler. Note though that most tutorials will only contain code and no configuration. So most of the time we don't need the Scala CLI/sbt tab. In this regard, the testing tutorial is different.

One thing that I need to improve though, is to show the Scala CLI tab by default.

Then we can avoid having to explain the files structure, we just say that you create that file and it could already have the using directive on top.

Do you mean we just say to create a file, and that file contains the //> using target.scope "test" directive? Is this really the recommended way?

There are at least 3 ways to create a test file in Scala CLI: the scope directive, the test folder and the .test.scala extension.

To me the .test.scala solution seems to be the best. It is easy to remember, and your files will be well organized and easy to navigate. In contrast, the //> using target.scope "test" is hard to understand, hard to remember and it does not tell you where to put the file or how you should name it.

As another option aside from the default scala-cli we could maybe have a short hyperlink on other options for installing the library.

I don't see how the hyperlink would be better than the dropdown. In both cases, the Scala CLI users can ignore it because they probably know already about the //> using toolkit directive. For the others, the dropdown seems better because they don't need to switch to a new page. They go directly from Goolge to any page of the tutorials and they have all the information they need, with no redirection.

I believe that we also could remove the explanation of the failing test log, as it seems pretty readable on its own.

Agreed.

In my opinion we could add value & perceived simplicity if we tried to make it more concise

Sure but also we should not try to make it look simpler than it is. Too simplistic examples with no explanation will create more questions than answers. Where to draw the line between conciseness and over-simplification? There are things that we know implicitly but are far from obvious to other people. While other things can more easily be figured out from the context.

In this example, what are the not so obvious things:

  • where to write the tests: in a test suite, in a test file
  • how to organize the test suites: one for each file or class
  • a test suite should be a class with an empty constructor
  • how to run the tests

The maybe self-evident things:

  • how to read the output of a failing test

@adpi2
Copy link
Member Author

adpi2 commented Nov 16, 2022

I think also it is worth noting that people will go to random pages of the Toolkit tutorials while Googling.

The Installing MUnit dropdown makes it clear that we use MUnit and not some other library. That's why I think it should be present on every pages.

For instance:
image

@szymon-rd
Copy link
Collaborator

szymon-rd commented Nov 16, 2022

I want to keep the tabs. They show clearly that we are trying not to divide the community. Also people can easily compare and see that the Scala CLI solution is simpler. Note though that most tutorials will only contain code and no configuration. So most of the time we don't need the Scala CLI/sbt tab. In this regard, the testing tutorial is different.

I agree. If the Scala CLI tab is default and short, then it shouldn't be much of a problem to have these different tabs available.

Do you mean we just say to create a file, and that file contains the //> using target.scope "test" directive? Is this really the recommended way?

The tests are a bit different indeed. In some other examples, it may be a good option to just write the snippets as if they were scripts with maybe some information present on how to integrate it into a bigger project. In some cases, the self-contained scala-cli scripts may be useful, as long as the reader has the knowledge how to use the solution under different circumstances or knows where to find this information quickly.

Sure but also we should not try to make it look simpler than it is. Too simplistic examples with no explanation will create more questions than answers. Where to draw the line between conciseness and over-simplification?

I agree, that's a problem where we have to strike the right balance. What I would try to avoid however is introducing many kinds of visually emphasized elements. Seeing the banner on top, a few different types of code snippets, the tabs and the red notice made me lost a bit and I think that other people may feel like it's a bit chaotic too. I think though that removing the failing test output snippet and making the red notice a standard text may help it.

@szymon-rd
Copy link
Collaborator

I think that in the tutorial describing how to run a single test we should describe testOnly. I think it's the easiest way, and it also works across testing libraries so the user may be happy that he already knows it from the past and has experience with it.

@adpi2
Copy link
Member Author

adpi2 commented Nov 17, 2022

I think that in the tutorial describing how to run a single test we should describe testOnly.

Yes that is the plan. The problem is that --only is not yet implemented in Scala CLI: VirtusLab/scala-cli#1226

@szymon-rd
Copy link
Collaborator

I talked about it with scala-cli team, it should be implemented soon.

@SethTisue
Copy link
Collaborator

curious about mdoc for the Scala 2 and 3 tabs — in scripts/run-mdoc.sh I only see Scala 2. are the Scala 3 tabs checked?

@bishabosha
Copy link
Member

curious about mdoc for the Scala 2 and 3 tabs — in scripts/run-mdoc.sh I only see Scala 2. are the Scala 3 tabs checked?

Not unless we can figure how to either extend mdoc with a scala 3 modifier , or split markdown files with some regexes

@adpi2
Copy link
Member Author

adpi2 commented May 5, 2023

I added the Toolkit in the index and navbar. Let me know what you think:

toolkit-index

@szymon-rd
Copy link
Collaborator

Perfect! Are we ready to deploy this?

@adpi2
Copy link
Member Author

adpi2 commented May 5, 2023

I also added some boxes in the intro:

image

@adpi2
Copy link
Member Author

adpi2 commented May 5, 2023

Perfect! Are we ready to deploy this?

I have nothing else to add. Should I open the PR to the real docs.scala-lang now?

@bishabosha
Copy link
Member

bishabosha commented May 5, 2023

I have nothing else to add. Should I open the PR to the real docs.scala-lang now?

I'm not sure about this until VirtusLab/scala-cli#2060 is fixed and released - otherwise munit wont work (with using toolkit "latest")

@adpi2
Copy link
Member Author

adpi2 commented May 5, 2023

I'm not sure about this until VirtusLab/scala-cli#2060 is fixed and released - otherwise munit wont work (with using toolkit "latest")

That's why I pushed this temporary workaround: a587ace

@bishabosha
Copy link
Member

something weird here:
Screenshot 2023-05-05 at 17 39 49

@szymon-rd szymon-rd self-requested a review May 5, 2023 16:05
Copy link
Collaborator

@szymon-rd szymon-rd left a comment

Choose a reason for hiding this comment

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

I think we can go ahead and make a PR to the scala-lang!

scala-improvement-bot and others added 3 commits May 6, 2023 00:19
* Add missing redirection from https://docs.scala-lang.org/scala3/book/types-type-classes.html to https://docs.scala-lang.org/scala3/book/ca-type-classes.html

* Follow the standard way of labelling a page to be Scala 3 only

* Mark “Extension Methods” as Scala 3 specific, and add cross-references with “Implicit Classes”.

* Add Scala 2 tabs and explanations in the Implicit Conversions page

Remove the detailed explanation from the Tour page in favor of the Book.

* Add code tabs to page 'Context Parameters'

* Add tabs to Type Classes page

* Update intro

* Address feedback from review
@adpi2
Copy link
Member Author

adpi2 commented May 9, 2023

Here is the final PR: scala#2795

@adpi2 adpi2 closed this May 9, 2023
@SethTisue SethTisue deleted the toolkit branch May 9, 2023 21:03
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.