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

Removes "write a program" from descriptions to address #321 #528

Merged
merged 56 commits into from
Feb 1, 2017

Conversation

robkeim
Copy link
Contributor

@robkeim robkeim commented Jan 30, 2017

@kotp
Copy link
Member

kotp commented Jan 30, 2017

I like it. It is all the same thing though, so could be one commit.

@robkeim
Copy link
Contributor Author

robkeim commented Jan 30, 2017

Thanks @kotp!

I would have done one commit but from this PR it seems we favor commits not crossing exercises. Whoever merges it can squash if they only want to keep one commit.

@kotp
Copy link
Member

kotp commented Jan 31, 2017

That PR and the suggestion for 3 different commits was for a different reason. The change here is one problem, and does not have the potential to effect like the other could.

@petertseng
Copy link
Member

petertseng commented Jan 31, 2017

Yeah I'd have to think about that one. I don't think I'd ever revert part of this PR but not the others, but I certainly won't complain about them being separate.

could you amend some commit messages? cryto -> crypto, kindergarten-garten -> kindergarten-garden, otcal -> octal, and serach -> search.

@robkeim
Copy link
Contributor Author

robkeim commented Jan 31, 2017

Thanks for the review @petertseng. I reviewed the changes before submitting but not the commit messages. I'll update them this evening.

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

nice, thanks for going through all of these.

output the notes in the given scale, starting with the tonic and
following the specified interval pattern.
Generate musical scales accepting a tonic, or starting note, and
a set of intervals. It should be able to output the notes in the
Copy link
Member

Choose a reason for hiding this comment

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

Previously, "it" referred to the program, and now I'm not sure "it" makes sense any longer. Maybe it will make sense by simply removing the words "It should"?

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 decided to reword the whole sentence to the "Given a xxx" syntax.

@@ -1,4 +1,4 @@
---
blurb: "Write a program that, given a word and a list of possible anagrams, selects the correct sublist."
blurb: "Given a word and a list of possible anagrams, selects the correct sublist."
Copy link
Member

Choose a reason for hiding this comment

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

probably selects -> select

@@ -1,4 +1,4 @@
---
blurb: "Write a program that implements a binary search algorithm."
blurb: "Implements a binary search algorithm."
Copy link
Member

Choose a reason for hiding this comment

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

Probably "Implement" here, so that the verb is consistent with other blurbs.

@@ -1,4 +1,4 @@
---
blurb: "Write a program that outputs the lyrics to 'The Twelve Days of Christmas'"
blurb: "Outputs the lyrics to 'The Twelve Days of Christmas'"
Copy link
Member

Choose a reason for hiding this comment

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

Probably "Output" here, to match verbs.

@kotp
Copy link
Member

kotp commented Jan 31, 2017

@petertseng is right, the same concern. I would not expect a portion of these to want to be reverted. It isn't a big deal about the individual commits, except a lot of short-interval work. And as noted, lots of opportunity for small errors in typing.

@robkeim
Copy link
Contributor Author

robkeim commented Jan 31, 2017

@petertseng I went ahead and updated the PR with your comments, thanks again for the review

@kotp I left the individual commits for now, but I could squash them with the merge, I don't have a preference.

@kotp
Copy link
Member

kotp commented Jan 31, 2017

Either way is good for me. With a squash, the commit will read really well, itemized.

@petertseng
Copy link
Member

Somehow, we lost the change commit, so that now the exercise's description again reads "Write a program"

also, can we get grade-school too? It says "Write a small archiving program"

@robkeim
Copy link
Contributor Author

robkeim commented Feb 1, 2017

Thanks @petertseng for all of the valuable feedback!

It looks like the Travis CI builds are backed up, so I'll merge this once the build completes.

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.

3 participants