Skip to content

Conversation

@eparovyshnaya
Copy link
Contributor

  • stub file for "Two Fer" task solution is appended
  • tests are un-ignored, as they got compilable over the main source base

The stub is questionable if the following aspects.

It contains hints to study about

It does not contain a clue for default arguments. I just do not know how to mention it withount giving a precise solution.

It is quite a Kotlin-like, but I'm not sure if it fits Exercism studying methodics.

Do you wish the stub to stay as close to the reference solution as possibl?
Do you want to avoid distraction by extra study motivators (like internal keyword and = sign in a function signture)?
If you do, I will rework the stub for sure.
If you don't, I have anoher proposal: can I add these links in the function's kotlin-doc?

@SleeplessByte
Copy link
Member

SleeplessByte commented Oct 8, 2019

Why are the tests un-ignored? Only the first test should be enabled.

IMO, the stub does not need to compile, but I think it's find to give a stub without a default argument. It's a pretty natural path to add a default argument as you go.

As for the rest:

  • I would remove internal.
  • I would use a regular function instead of =.

@eparovyshnaya
Copy link
Contributor Author

All questionable things are undone.

We stll cannot get this compiled until PR 318 is merged.

@SleeplessByte
Copy link
Member

Yes. @uzilan mentioned it on the Slack :)! Thanks for your work @eparovyshnaya 🎉

@SleeplessByte
Copy link
Member

If you don't, I have anoher proposal: can I add these links in the function's kotlin-doc?

If you show me what that would look like... sure :)

@SleeplessByte
Copy link
Member

Nice!

Don't use KDoc for this, but the TrackInsert hint! Then, readme needs to be re-generated. The hint will become part of README.md.

Yes, use TODO!! That's a great fit.

@eparovyshnaya
Copy link
Contributor Author

Got it. Will remove the KDocs from both stubs to call it a day for this PR. Will open another one for TrackHints.

Copy link
Member

@SleeplessByte SleeplessByte left a comment

Choose a reason for hiding this comment

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

@exercism/kotlin please merge 🤗

@dector dector self-assigned this Nov 11, 2019
@dector dector self-requested a review November 11, 2019 19:47
Copy link
Member

@dector dector left a comment

Choose a reason for hiding this comment

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

👍 Please, squash into single commits and merge.

@dector dector removed their assignment Nov 11, 2019
@SleeplessByte
Copy link
Member

@dector if you squash-merge here, those steps are unnecessary.

@dector
Copy link
Member

dector commented Nov 12, 2019

@SleeplessByte Kinda. I'm not using built-in functionality here intentionally.
I'm not sure how it handles commit author info (especially, for signed commits). There is some open issues. I'm just not familiar how and what this merge mode does behind the curtain. :)

There is no rush for this PR so I suggested to do it manually.

@eparovyshnaya
Copy link
Contributor Author

Guys, I do appreciate your help and care. Just let me sort it out. Taming this rebase became an affair of honor for me.

@eparovyshnaya
Copy link
Contributor Author

@dector, made it. Can you please merge?

@dector dector merged commit 6e7de6f into exercism:master Nov 12, 2019
@eparovyshnaya eparovyshnaya deleted the 310_stubs branch November 12, 2019 11:08
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