Skip to content

Rewrote compound types tour #742 #988

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 2 commits into from
Jan 16, 2018
Merged

Rewrote compound types tour #742 #988

merged 2 commits into from
Jan 16, 2018

Conversation

mghildiy
Copy link
Contributor

No description provided.

@mghildiy
Copy link
Contributor Author

PR 'Rewrote compound types tour #742' can be closed.

@@ -24,12 +22,12 @@ trait User {
}

trait Tweeter {
this: User => // reassign this
def tweet(tweetText: String) = println(s"$username: $tweetText")
this: User => // reassign this

Choose a reason for hiding this comment

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

Is there a reason this was changed from two spaces to a tab? I'd prefer keeping two-space indents.

Copy link
Member

@jvican jvican 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're missing some of the changes in https://github.com/scala/docs.scala-lang/pull/742/files, did you rebase from https://github.com/travissarles/scala.github.com/tree/compound-types? If you use hub, you can fetch Travis's branches by doing git fetch travissarles and then doing a rebase for that branch. You can also copy-paste them if you want 😄.

def hasNext: Boolean
def next(): A
}

Copy link
Member

Choose a reason for hiding this comment

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

Could we remove this extra space? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume by extra space you mean line #41.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or extra space in 38,39.

_tour/traits.md Outdated
@@ -50,13 +54,13 @@ class IntIterator(to: Int) extends Iterator[Int] {


val iterator = new IntIterator(10)
println(iterator.next()) // prints 0
println(iterator.next()) // prints 1
iterator.next() // prints 0
Copy link
Member

Choose a reason for hiding this comment

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

// return 0 rather than // prints 0?

Copy link
Member

@jvican jvican left a comment

Choose a reason for hiding this comment

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

Oops, my bad! Just realised those changes were already checked in. They must have been added in another PR. Thank you for your work. Can you address the previous comment and we merge? 🚀

@mghildiy
Copy link
Contributor Author

I have replaced the tab with two spaces.

@jvican
Copy link
Member

jvican commented Jan 14, 2018

@mghildiy If you're looking for more, we still need one more contributor in #746 😄.

@mghildiy
Copy link
Contributor Author

mghildiy commented Jan 14, 2018 via email

@jvican
Copy link
Member

jvican commented Jan 14, 2018

Absolutely not, take your time. That's one of the gems that can actually be enlightening to work on if you're new to Scala; a solid understanding of implicits is paramount to become a better Scala programmer..

@jvican
Copy link
Member

jvican commented Jan 14, 2018

Let me know when you've addressed these comments and we merge @mghildiy.

@mghildiy
Copy link
Contributor Author

mghildiy commented Jan 14, 2018 via email

@jvican
Copy link
Member

jvican commented Jan 15, 2018

It seems you forgot to address my review comments though?

@mghildiy
Copy link
Contributor Author

Changes done

@jvican jvican merged commit f745d16 into scala:master Jan 16, 2018
@SethTisue
Copy link
Member

thank you @mghildiy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants