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 exercise complex-numbers #523

Merged
merged 7 commits into from
Sep 8, 2017

Conversation

lilislilit
Copy link

Trying to implement complex-numbers according to guideline. This is initial commit for WIP pull-request. Tests and example.py still needs to be finished.

@ilya-khadykin
Copy link
Contributor

Good luck with this one and have fun 👍 @lilislilit

@lilislilit lilislilit changed the title [WIP] Implement exercise complex-numbers Implement exercise complex-numbers Sep 8, 2017
@lilislilit
Copy link
Author

tests and example.py are both finished, so there is that. Humbly waiting for nitpicking and/or approval.

Copy link
Contributor

@ilya-khadykin ilya-khadykin left a comment

Choose a reason for hiding this comment

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

Looks good!
Only some small things can be improved further

config.json Outdated
"uuid": "7f4d5743-7ab8-42ca-b393-767f7e9a4e97",
"slug": "complex-numbers",
"core": false,
"unlocked_by": "leap",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please share with us the reasoning behind this decision?
Don't get me wrong, I am not against it, just curios

And it turned out (while testing a new version of Exercism) that non-core exercise should be unlocked by only a core one.
We should revisit the whole track curriculum, perhaps you can help with a fresh view

Copy link
Author

Choose a reason for hiding this comment

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

To be completely honest, I was looking at a c# track, it looks somewhat finished, so I figured that their unlocking graph is finished too. If we will be redoing the whole of dependency graph, maybe it warrants separate issue and separate PR?

Copy link
Contributor

@ilya-khadykin ilya-khadykin Sep 8, 2017

Choose a reason for hiding this comment

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

Yeah, this should be addressed in a separate issue\PR.
I didn't have a chance to create it, I'll create it later today
But still, it's better to leave unlocked_by empty for now (until we'll figure out a list of core exercises)

config.json Outdated
"slug": "complex-numbers",
"core": false,
"unlocked_by": "leap",
"difficulty": 6,
Copy link
Contributor

Choose a reason for hiding this comment

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

The same for difficulty.
What affected the score to be 6?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, same hear. Took it from C# track. As I understood, difficulty grade is kinda... subjective.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Maybe we can come with some sort of formula to compute it, what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I think this question maybe hurried somewhere in general discussions repo. Btw, will difficulty be used at all in version 2, or there would be relying on "unlocked_by" prerequisites?

In general, it could be done, of course, simplest heuristic would be to split topics that we have in to tiers, from simplest to hardest and add the difficulty level for each additional topic for exercise. So if basically tier_level1+....tier_level2

Copy link
Contributor

Choose a reason for hiding this comment

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

@lilislilit, hm, this a really good point, thanks! I haven't thought about it this way
I've created the following issue exercism/discussions#193 in general discussion, feel free to add your thoughts there

config.json Outdated
"unlocked_by": "leap",
"difficulty": 6,
"topics": [
"Tuples",
Copy link
Contributor

Choose a reason for hiding this comment

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

Good choice.

We are trying to standardize topics across all tracks and decided to use snake_case notation as a naming convention. List of of common topics can be found here - https://github.com/exercism/problem-specifications/blob/master/TOPICS.txt
Could you please convert them to 'snake_case'

@@ -0,0 +1,3 @@
class ComplexNumber(object):
def __init__(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

As it was discussed in #509 it would be a good idea to include expected parameters to not force the user to go through a test class

What are your thoughts on this?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I think it is a wise thing to do

Copy link
Author

Choose a reason for hiding this comment

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

Although, should I also include method signatures? Or only params for constructor? If we don't want people to go through test suite too much, including method signatures makes sense, some tracks here do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it makes sense to include method signatures since the user uses the provided test suite anyway.
And if someone wants to create their own unique solution, they are free to change provided class placeholder or even test suite (rewrite it, add or remove tests)

I don't see how providing method signatures can influence the user's overall implementation. The most important thing for me is convenience

… added write-up for ComplexNumber template class
@ilya-khadykin
Copy link
Contributor

@lilislilit, everything looks good to me, thanks for taking the time and implementing this!

Generally it would be a good idea to squash commits, but it's up to you

@lilislilit
Copy link
Author

lilislilit commented Sep 8, 2017

@m-a-ge From the looks of history, commits look squashed :) Also, thank you for your time. Looking forward to do some more stuff for this track :octocat:

@ilya-khadykin ilya-khadykin merged commit 5ad7cbd into exercism:master Sep 8, 2017
@cmccandless
Copy link
Contributor

Test cases test_multiply_numbers_with_real_and_imaginary_part is incorrect. Please stick to the canonical data when implementing established exercises in a new language.

@ilya-khadykin
Copy link
Contributor

@cmccandless could please create an issue describing the details?

@cmccandless
Copy link
Contributor

Done. See #527.

@ilya-khadykin
Copy link
Contributor

@cmccandless thanks!

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