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

Connect use json #335

Merged
merged 5 commits into from
Jul 31, 2016
Merged

Connect use json #335

merged 5 commits into from
Jul 31, 2016

Conversation

jonatas
Copy link
Contributor

@jonatas jonatas commented Jul 27, 2016

I updated from static examples to use from connect.json on x-common project.

It was merged recently: exercism/problem-specifications#297.

@petertseng
Copy link
Member

Everything here looks OK.

Probably would be good if commit messages all mention "connect" somewhere.

I added two cases to connect.json

I am wondering whether this exercise should get a testVersion now, since solutions made before this PR will not pass the test suite after this PR.

(testVersion like https://github.com/exercism/xgo/blob/master/exercises/leap/leap.go and https://github.com/exercism/xgo/blob/master/exercises/leap/leap_test.go )

// Source: exercism/x-common
// Commit: 6c6a395 improving description again

const testVersion = 2
Copy link
Member

Choose a reason for hiding this comment

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

we would want the testVersion to go in example.go so that submitted solutions have to show their testVersion (allowing reviewers of the code to easily see what version of the tests a solution was coded against). Putting it in cases_test.go would mean that nobody has to declare it

@petertseng
Copy link
Member

I'm fine with everything I see here, and I asume if anyone else had anything to say I expect they should have said something earlier since this PR has been here for a few days.

Some final things to check on, and then I'll merge. This will happen within 24 hours, and you can and should feel justified in prodding me if I haven't done that.

Note: I plan to squash all commits into one before I merge, since this only really makes sense as one commit.

@petertseng petertseng merged commit e07848c into exercism:master Jul 31, 2016
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