Skip to content

Lower bound in tour includes dangers #2355

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

Merged
merged 3 commits into from
Apr 1, 2022
Merged

Conversation

som-snytt
Copy link
Contributor

Challenging to fit on a single tour slide, but tries to emphasize that there are two concepts in play: covariance and need for lower bound. The naming of the list example classes may or may not be a distraction.

@julienrf
Copy link
Contributor

Thank you @som-snytt, however, I think it would be better to minimize the changes between the code snippet that does not contain a lower bound and the one that showcases a lower bound, to make it easier for the readers to understand the scope of type bounds. I suggest using Nothing and head and tail from the beginning, instead of doing this change here. What do you think?

@som-snytt
Copy link
Contributor Author

The suggestion on the linked PR was that the naming of "List" was potentially confusing.

If I'm allowed to muck with the entire example, is it OK to make it look like the familiar List and avoid the nerdy Node terminology?

I was trying avoid changing too much, in case there are pedagogical reasons.

@julienrf
Copy link
Contributor

I am fine with removing the Node terminology.

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! I have some suggestions.

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, sorry for the additional work :) I think this is good to merge, I just left one minor suggestion.

@julienrf julienrf merged commit 1d643ff into scala:main Apr 1, 2022
@som-snytt
Copy link
Contributor Author

@julienrf thanks for making it better

@som-snytt som-snytt deleted the review/2347 branch April 1, 2022 08:52
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.

2 participants