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

hello world tests via updated x-common data #550

Merged
merged 1 commit into from
Apr 16, 2017
Merged

Conversation

kotp
Copy link
Member

@kotp kotp commented Feb 20, 2017

Description

Uses the x-common data to simplify the hello world introductory exercise and tests.

Motivation and Context

re: exercism/problem-specifications#520

  • All new and existing tests passed.

@kotp kotp added the ready label Feb 20, 2017
Copy link
Contributor

@tommyschaefer tommyschaefer left a comment

Choose a reason for hiding this comment

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

Should the example implementation be updated to remove the Hello, Foo functionality?

Not sure if any of these are really that important, just things that I noticed 😄.

class HelloWorldTest < Minitest::Test
def test_no_name
def test_Say_Hi!
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this maybe be downcased to test_say_hi?

2
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the version need to be bumped since the old implementation is still valid?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I believe so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Clarification on why I believe it needs to be incremented is that old version would take an argument, this new version will not accept an argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the version increment.

@kotp kotp force-pushed the hello-world_Simplified branch from 2c3a6c3 to 72f5d93 Compare March 17, 2017 08:05
@kotp
Copy link
Member Author

kotp commented Mar 17, 2017

nb: Generator still uses git commit sha for version, perhaps should use canonical data version instead. @Insti @tommyschaefer ?

@Insti
Copy link
Contributor

Insti commented Apr 16, 2017

Generator should be using the sha1 from the canonical data repository already.

@Insti Insti merged commit 1a02a7a into master Apr 16, 2017
@Insti Insti removed the ready label Apr 16, 2017
@Insti
Copy link
Contributor

Insti commented Apr 16, 2017

Generator should be using the sha1 from the canonical data repository already.

Now I've caught up a bit more I see you mean the version property that is now present in the canonical-data.json files. I'll need to think about the implications of this. Will continue the discussion in: #485

@kotp kotp deleted the hello-world_Simplified branch April 23, 2017 02:56
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