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

Implement new Concept Exercise: tuples #1009

Merged
merged 36 commits into from
Nov 2, 2021

Conversation

Grenkin1988
Copy link
Contributor

Fixws #863

@Grenkin1988 Grenkin1988 marked this pull request as ready for review October 23, 2021 11:51
@ErikSchierboom
Copy link
Member

I'll look at this tomorrow! Thanks.

concepts/tuples/.meta/config.json Outdated Show resolved Hide resolved
concepts/tuples/introduction.md Outdated Show resolved Hide resolved
concepts/tuples/links.json Outdated Show resolved Hide resolved
concepts/tuples/links.json Outdated Show resolved Hide resolved
exercises/concept/tisbury-treasure-hunt/.docs/hints.md Outdated Show resolved Hide resolved
exercises/concept/tisbury-treasure-hunt/.docs/hints.md Outdated Show resolved Hide resolved
exercises/concept/tisbury-treasure-hunt/.docs/hints.md Outdated Show resolved Hide resolved
@ErikSchierboom
Copy link
Member

@Grenkin1988 I've left some comments, but before you look into any of them, I feel like we should first discuss #1009 (comment), as that is key to this exercise.

@Grenkin1988
Copy link
Contributor Author

@ErikSchierboom Hopefully I've covered all of the comments.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Looking good! I'll comment on the various markdown documents later. I focused on the other files first.

exercises/concept/tisbury-treasure-hunt/.meta/Exemplar.fs Outdated Show resolved Hide resolved
exercises/concept/tisbury-treasure-hunt/.meta/config.json Outdated Show resolved Hide resolved
let getCoordinate (line: string * string): string = snd line

let convertCoordinate(coordinate: string): int * char =
Int32.Parse(string coordinate.[0]), coordinate.[1]
Copy link
Member

Choose a reason for hiding this comment

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

I hadn't considered the fact that we need students to convert a char to an int, which they probably don't know how to do. I do think it's worth keeping an int to really stress the "different types" feature of a tuple. Could you perhaps add a suggestion how to do this to the hints.md file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also might be worse adding parsing to the strings concept?
For example we could have some "EventId=1356" in the log message, and as last task to get EventId as an int?

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 not sure, I'll think on it.

Grenkin1988 and others added 5 commits October 27, 2021 15:41
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
concepts/tuples/about.md Outdated Show resolved Hide resolved
concepts/tuples/about.md Outdated Show resolved Hide resolved
concepts/tuples/about.md Outdated Show resolved Hide resolved
concepts/tuples/introduction.md Outdated Show resolved Hide resolved
(a + 1, b + 1)
```

## Obtaining Individual Values
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice to move the examples next to the text where the concept is explained. For example, the example of matching a tuple can be moved to the pattern matching example, possibly including the deconstruction code too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, not sure I understand what is required

Copy link
Member

Choose a reason for hiding this comment

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

No problem. I can do a follow-up PR later.

Grenkin1988 and others added 2 commits October 28, 2021 21:41
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Grenkin1988 and others added 12 commits October 28, 2021 21:41
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
@ErikSchierboom
Copy link
Member

@Grenkin1988 Happy with the current state of the PR? If so, I can merge this and tweak the introduction phrasing slightly in a follow-up PR (easier that modifying in this PR).

@Grenkin1988
Copy link
Contributor Author

I guess so

@ErikSchierboom ErikSchierboom added the x:size/large Large amount of work label Nov 2, 2021
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Let's go! Awesome work @Grenkin1988! 🎉

@ErikSchierboom ErikSchierboom merged commit 41f85ab into exercism:main Nov 2, 2021
@Grenkin1988 Grenkin1988 deleted the tuples-concept-create branch November 2, 2021 10:06
@Grenkin1988
Copy link
Contributor Author

This was really interesting experience 😸 Hope it wasn't way to terrible 😅

@ErikSchierboom
Copy link
Member

No not at all! Building Concept Exercises is quite a lot of work, so some churn is to be expected :) You did great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:size/large Large amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants