Skip to content

meetup: Clarify description #982

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 10 commits into from
Nov 8, 2017

Conversation

Insti
Copy link
Contributor

@Insti Insti commented Oct 30, 2017

Clarify the problem description for meetup.

This PR is made up of several commits:

  • Enumerate valid descriptors.
  • Lowercase '*teenth' descriptors.
  • Add description of what to do if there is no date.
  • Note that all descriptions will be valid.
  • Make descriptions consistent.
  • Reformat text to fit in 80 columns.

The goal here is to make the description less ambiguous by explicitly stating the expected date specifier strings and what to do if there is no matching date.

A consistent format for the input date specifications and only testing against valid input strings helps ensure that this remains a problem about date parsing rather than arbitrary string parsing.

The final commits reformat the text to 80 columns for consistency with other exercise descriptions.

"The first Monday of January 2017", the correct meetup date is 2017/1/2

All descriptions will be valid, but you will need to return an error if there
is no matching date. For example, "The fifth Wednesday of October 2017"
Copy link
Member

Choose a reason for hiding this comment

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

It feels like the last line is missing something at the end. Maybe add " has no matching date"?


All descriptions will be valid, but you will need to return an error if there
is no matching date. For example, October 2017 has five Tuesdays but only four
Wednesdays. There is no date that matches "The fifth Wednesday of October 2017"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we even be testing with invalid dates?
We're not testing other invalid inputs.

I suggest removing the last section completely and leaving error cases undefined by the canonical data.

@Insti
Copy link
Contributor Author

Insti commented Nov 2, 2017

e5dea5b - Clarifies the invalid date handling description.

2b4cfb6 - Leaves invalid date handling undefined.

For example, if given "First Monday of January 2017", the correct meetup date is 2017/1/2
Given examples of a meetup dates, each containing a month, day, year, and
descriptor calculate the date of the actual meetup. For example, if given
"The first Monday of January 2017", the correct meetup date is 2017/1/2
Copy link
Member

Choose a reason for hiding this comment

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

I'm missing a dot (.) at the end of the line, or was that on purpose?.

Copy link
Member

Choose a reason for hiding this comment

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

@Insti small bump for my comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I needed the bump. 👍

@ErikSchierboom
Copy link
Member

Great improvement!

@ErikSchierboom
Copy link
Member

@Insti Would you like to keep the various commits or should I do a squash?

@Insti
Copy link
Contributor Author

Insti commented Nov 8, 2017

Personally, I'd keep them, since they are all atomic actions and I prefer to be able to see all the change steps in the git history.
Other people like squashing for some reason, so do whatever you think is best.

@ErikSchierboom ErikSchierboom merged commit e93924e into exercism:master Nov 8, 2017
@ErikSchierboom
Copy link
Member

Merged! Thanks a lot @Insti. 🎉

@Insti Insti deleted the Meetup_clarification branch November 8, 2017 09:12
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