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

Add exercise: phone-number #91

Merged
merged 1 commit into from
Oct 19, 2017
Merged

Add exercise: phone-number #91

merged 1 commit into from
Oct 19, 2017

Conversation

Teo-ShaoWei
Copy link
Contributor

@Teo-ShaoWei Teo-ShaoWei commented Oct 18, 2017

This is a call-to-arm to issue #87. First time here, pardon me if anything is out of spec. Feel free to inform me on how I can polish it and I will get it fixed.

Some design decisions...

  • The main machinery of this question is regex. Instead of mixing up regex with language specifics like some other language tracks, I decided to put the full power on regex. Instead the example demonstrates how Julia can call regex, and create regex using interpolation to improve code readability.
  • I did not put in type declaration for the input variable, pending discussion on issue Consider consistency for the exercises #2. I will adhere to the convention of this repo after we concluded the discussion.

Copy link
Contributor

@SaschaMann SaschaMann left a comment

Choose a reason for hiding this comment

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

This looks good to me. That said, I don't know anything about regex, so I can't say much about the example solution :D

I would leave this as a floating non-core exercise. Understanding regex is not really important in many applications of Julia, so I don't think it should be part of the main progression line in exercism v2. Unless you have another suggestion.

exchange = "($NXX)"
subscriber = "([\\d]{4})"
fillers = "\\s*(?:\\.|-)?\\s*"
r = Regex("^\\s*$country$fillers$area$fillers$exchange$fillers$subscriber\\s*\$")
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use regex string literals here, might look a bit neater.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I would have preferred to demonstrate regex string literals but I think it is not possible to do regex interpolation (refer to this issue). At the end of it the suggested solution is to use the Regex constructor (as per this discourse). The string looks more unwieldy without interpolation, so I took the lesser of the two evils.

Another way is to extract all the digits first and then check only the digits, but there will be a lot of false positives.

I am not good with regex myself too actually haha... I think the test cases are ok, so we can consider publish it first, and then we might have a submission that solve this nicely =)

Copy link
Contributor

@SaschaMann SaschaMann Oct 19, 2017

Choose a reason for hiding this comment

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

Ah okay, I didn't know of that issue.

@Teo-ShaoWei
Copy link
Contributor Author

Thanks for the review @SaschaMann =)

Yupz I agree with you that this would be a floating non-core exercise. I think I left the config.json entry of this problem as "core": false,. Is that the correct setting?

Exercism v2? That's new to me. Where can I find out more about it?

@SaschaMann
Copy link
Contributor

Yep, that's the correct setting.

Info about exercism v2 is spread out over the discussion and docs repos. One of the major changes is the progression tree as opposed to the current, linear progression.

Some info:
exercism/discussions#175
exercism/discussions#159
exercism/discussions#154

The prototype/beta is available here: v2.exercism.io (Julia isn't deployed on that yet)

@SaschaMann SaschaMann merged commit f469c7d into exercism:master Oct 19, 2017
@SaschaMann
Copy link
Contributor

Thanks for adding this! :)

@Teo-ShaoWei
Copy link
Contributor Author

Ah I will give Exercism v2 info a good read. It will be good if we can prepare for it while we import questions.

Thanks for reviewing too. I look forward to working with you and the rest of the team on other imports =)

@Teo-ShaoWei Teo-ShaoWei deleted the ex-phone-number branch October 19, 2017 14:23
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