Skip to content

Add code tabs to Other changes page #2725

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 5 commits into from
Mar 13, 2023
Merged

Conversation

Dedelweiss
Copy link
Contributor

@Dedelweiss Dedelweiss commented Mar 8, 2023

Here is my PR to add code tabs to the Other changes page

Ref #2481

Copy link
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

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

Hey Lucas, I started reviewing this but it seems you always put “Scala 3 Only” as a tab title for code that compiles with Scala 2 only. Or do I miss something?

@Dedelweiss
Copy link
Contributor Author

Hey Lucas, I started reviewing this but it seems you always put “Scala 3 Only” as a tab title for code that compiles with Scala 2 only. Or do I miss something?

Oh yes, that's a misunderstanding on my part. For me, since there was an error afterwards it was like saying that this code in scala 3 has the error right afterwards. So what I can do is put the code in scala 2 only and say that in scala 3 only it produces the error.

@julienrf
Copy link
Contributor

julienrf commented Mar 8, 2023

what I can do is put the code in scala 2 only and say that in scala 3 only it produces the error.

That sounds good to me, thank you!

@Dedelweiss
Copy link
Contributor Author

Dedelweiss commented Mar 9, 2023

When there is an error in the code itself as in the example below. This means that in Scala 3 there is an error so should I still put in Scala 2 only?
Screenshot 2023-03-09 at 09 43 43

@julienrf
Copy link
Contributor

julienrf commented Mar 9, 2023

Yeah, that is tricky, I understand your reasoning. By putting the “Scala 3 Only” you mean “that error that we mention in the code snippet happens in Scala 3 Only”. But everywhere else on the website we use “Scala 3 Only” to indicate a code snippet that is valid only in Scala 3.

I think we could make things clearer with the following approach:

  • Put the “Scala 2 Only” tab to indicate that the code snippet is valid in Scala 2 only

  • In the comment where we show the error message reported when we compile that snippet with Scala 3, make it explicit that the error message is specific to Scala 3:

    pojo.setFooBar("hello") // In Scala 3, Not Found Error: value setFooBar is not a member of Pojo
  • Above the code example, make it explicit again that it is valid in Scala 2 but not in Scala 3:

    For instance, the below Scala 2 code would fail to compile in Scala 3:

    (instead of:)

    For instance, in the below code:

How does that sound?

@Dedelweiss
Copy link
Contributor Author

How does that sound?

Oh yes, indeed, I think it is better in terms of understanding. Thank you for your attention. And so I put back all the {% highlight diff %}? I thought it was a difference between Scala 2 and Scala 3 so I put them in the tabs, but it is true that in terms of understanding it is good to have the difference with the -/+. Should I still put the {% highlight diff %} in the tabs of Scala 3 only (or others)? Like this example below :

Screenshot 2023-03-09 at 11 41 25

@julienrf
Copy link
Contributor

julienrf commented Mar 9, 2023

In that case, since we show a diff and not a code snippet that the readers than copy-paste, we can leave out the tab.

Corrections of the tabs.

- Change scala 3 only by scala 2 only
- Re-add highlight parts

Delete Darwin dependencies
Copy link
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

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

Thank you again Lucas for your help, but I’ve noticed a pattern that should be fixed: the tabs should primarily wrap code snippets, not text, unless the tabs contain different code snippets and we want to add some text specifically for each snippet.

@Dedelweiss Dedelweiss requested a review from julienrf March 11, 2023 18:39
Copy link
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

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

Thank you for adding all the code tabs!

@julienrf julienrf merged commit 0513751 into scala:main Mar 13, 2023
@Dedelweiss Dedelweiss deleted the other_changes branch March 13, 2023 08:09
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants