search

lihaoyi / hands-on-scala-js

Better documentation for Scala.js
142 stars 52 forks source link

Integrate existing pull requests #43

Closed adamvoss closed 7 years ago

adamvoss commented 7 years ago

I noticed there were are a number of pull requests open and I wanted to be on the latest before continuing through myself, especially so I might not make the same correction someone already has made.

I reviewed all the changes and integrated them making changes as necessary to fix the build, respond to comments made on the PR, and follow good style for a book.

The only one that I somewhat hesitated on was #37, I think the correction is good, but if a different implementation should be used to be less cryptic to a new learner. However, as was I still thought it an improvement and it can always be changed again later.

I understand you may not trust my reviewing and editing skills to merge this outright; though, if you do merge this all existing PRs except #40 can be closed as merged.

sjrd commented 7 years ago

Do you think you could rebase this on top of master rather than merging things here and there? I'm not very comfortable with reviewing merge commits inside PRs.

Also could you squash "Fix build following changes in PR #34" with the commit that breaks the build?

adamvoss commented 7 years ago

Sure, I'll rewrite history and get back to you. I was hesitant to do that initially because they are not my commits so I wanted to be transparent that I was merging them and that in some cases the merges had modifications (as mentioned in the commit message). The end result of the code is the same though, so I will just append their original commit message where applicable to note what I have done.

adamvoss commented 7 years ago

@sjrd Done.

Interestingly but unrelated, I was writing a complaint and warning here that GitHub was listing the commits according to Author Date rather than Commit Date (which are not the same order in this case), but then after refreshing the page it is now showing by Commit Date.

sjrd commented 7 years ago

I'll be busy all weekend, but I'll review this on Monday. Should I fail to do that, feel free to remind me.

adamvoss commented 7 years ago

@sjrd Pinging you with a reminder as per request :-)