Skip to content
This repository has been archived by the owner on Aug 31, 2021. It is now read-only.

[C#] Add exercise User-defined Exceptions #1617

Merged
merged 22 commits into from
Jun 17, 2020
Merged

[C#] Add exercise User-defined Exceptions #1617

merged 22 commits into from
Jun 17, 2020

Conversation

mikedamay
Copy link
Contributor

  1. Please review UserDefinedExceptions.cs as part of your initial review of this draft.
  2. @ErikSchierboom I know you are averse to including language features in the test file that are not pre-requisites for the exercise. We need lambdas in the test file but I don't really want to increase the dependency count. I did half the Haskell track without really understanding how the test framework worked so I'm not so committed to this principle as you. However, I am happy to fall into line if necessary.
  3. I was in two minds about whether to include less boilerplate in the code provided for CalculationException but decided against it on the grounds that it could multiply the number of representations required. I can't really judge

@mikedamay mikedamay added status/in-progress This issue is being worked on type/new-exercise Add a new exercise labels Jun 12, 2020
mikedamay and others added 2 commits June 16, 2020 11:50
…ta/Example.cs

Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
@mikedamay
Copy link
Contributor Author

I have marked 4 suggestions A), B), C) and D) in the review comments. These all amount to the same thing to a greater or lesser extent. My choice is for simplicity. I don't think the proposed changes add much to the features of user defined exceptions exercised here although they make the code a little more natural and the story a bit more realistic.

I am happy to make the suggested changes but I don't see why you come down in favour of naturalness/realism rather than simplicity. Particularly as the whole thing is artificial in the first place. To effectively do reviews myself I need to know the criteria otherwise it will put more of a burden on you.

While we are at the draft review stage you can safely skip obvious "errors" like CalculationException_template if you want (although I'm always grateful to have typos picked up). This sort of review seems to be more about the big picture.

@ErikSchierboom
Copy link
Member

I have marked 4 suggestions A), B), C) and D) in the review comments.

I don't see them marked, but I assume you are referring to the four open comments?

While we are at the draft review stage you can safely skip obvious "errors" like CalculationException_template if you want (although I'm always grateful to have typos picked up). This sort of review seems to be more about the big picture.

It is. I just couldn't help myself :)

@mikedamay
Copy link
Contributor Author

It is. I just couldn't help myself :)

Fair enough. Certainly no problem for me.

I don't see them marked, but I assume you are referring to the four open comments?

Yes the open comments. You will see the letters there.

@ErikSchierboom
Copy link
Member

Yes the open comments. You will see the letters there.

I don't see them :)

@mikedamay
Copy link
Contributor Author

Sorry. did it again - ignored PENDING.

You should see them now.

@mikedamay mikedamay marked this pull request as ready for review June 17, 2020 06:56
@mikedamay mikedamay requested a review from a team as a code owner June 17, 2020 06:56
@mikedamay
Copy link
Contributor Author

yep - ready to review

mikedamay and others added 5 commits June 17, 2020 09:30
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
…cs/after.md

Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
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.

Brilliant! Thanks for adding this exercise!

@mikedamay
Copy link
Contributor Author

I think we should have sliced up the functionality differently putting the wrapping in the calculator class and the filtering in test harness. I think it would make marginally more sense. I may revisit it after we have done the first 42.

@mikedamay mikedamay merged commit 3945e09 into exercism:master Jun 17, 2020
@mikedamay mikedamay deleted the csharp/user-exceptions branch June 17, 2020 11:57
@ErikSchierboom
Copy link
Member

I think we should have sliced up the functionality differently putting the wrapping in the calculator class and the filtering in test harness. I think it would make marginally more sense. I may revisit it after we have done the first 42.

Ah that would have been nice indeed. Maybe you could create an issue for it? It sounds like this would be nice an an improve issue :)

@mikedamay
Copy link
Contributor Author

See #1664

BethanyG pushed a commit to BethanyG/v3 that referenced this pull request Jun 27, 2020
* csharp/user-exceptions initial upload

* csharp/user-exceptions initial code/instructions complete

* csharp/user-exceptions tweak to design.md

* csharp/user-exceptions proofing

* csharp/user-exceptions proofing

* csharp/user-exceptions after review

* csharp/user-exceptions after review

* csharp/user-exceptions intro and after

* csharp/user-exceptions tweaking docs

* Update languages/csharp/exercises/concept/user-defined-exceptions/.meta/Example.cs

Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>

* Apply suggestions from code review

Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>

* csharp/user-exceptions more review changes

* csharp/user-exceptions substantially complete

* csharp/user-exceptions removed text not required for publication

* csharp/user-exceptions added pre-requisites

* csharp/user-exceptions updated out-of-date instructions

* csharp/user-exceptions updated config

* Apply suggestions from code review

Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>

* Update languages/csharp/exercises/concept/user-defined-exceptions/.docs/after.md

Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>

* csharp/user-exceptions late review points

* Update after.md

* copy/paste error

Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/in-progress This issue is being worked on track/csharp type/new-exercise Add a new exercise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants