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

Introduce "what next instructions" to submission API response #3414

Merged
merged 2 commits into from Mar 20, 2017

Conversation

Projects
None yet
2 participants
@nilbus
Member

nilbus commented Mar 19, 2017

This is paired with exercism/cli#377 but does not need not need a synchronized release. This change forward/backward compatible, tested in all combinations of new & old exercism.io and new & old cli.

In clients that support it, the "what next instructions" will be displayed after a new iteration is submitted.

This allows us to modify the message anytime, without having to wait for users to update their cli version.

Part of the experiment outlined in exercism/discussions#123.

Introduce "what next instructions" to submission API response
In clients that support it, the "what next instructions" will be
displayed after a new iteration is submitted.

This allows us to modify the message anytime, without having to wait for
users to update their cli version.
@nilbus

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Mar 19, 2017

Member

Two things I'd be interested in feedback on:

  • Is there a better place than the route file to define the copy for the what next instructions? Rendering a view with locals seems like an option that may be overcomplicated or confusing, since it's not the response that is rendered.
  • Are you happy with the copy?

If this all looks good to you though, we can :shipit:.

Member

nilbus commented Mar 19, 2017

Two things I'd be interested in feedback on:

  • Is there a better place than the route file to define the copy for the what next instructions? Rendering a view with locals seems like an option that may be overcomplicated or confusing, since it's not the response that is rendered.
  • Are you happy with the copy?

If this all looks good to you though, we can :shipit:.

Match Rubocop TargetRubyVersion with .ruby-version (2.3.3)
Since the application refuses to run on anything other than the version
specified in .ruby-version and the Gemfile, there's no reason to
maintain support for earler versions.

Despite what the [Rubocop docs][] say, rubocop is targeting ruby 2.0
when TargetRubyVersion is unspecified.

[Rubocop docs]: https://github.com/bbatsov/rubocop/blob/master/manual/configuration.md#setting-the-target-ruby-version
@@ -1,7 +1,7 @@
inherit_from: .rubocop_todo.yml
AllCops:
- TargetRubyVersion: 2.2
+ TargetRubyVersion: 2.3

This comment has been minimized.

@kytrinyx

kytrinyx Mar 19, 2017

Member

Nice catch, thank you. I'm always missing these when we update.

@kytrinyx

kytrinyx Mar 19, 2017

Member

Nice catch, thank you. I'm always missing these when we update.

@@ -5,6 +5,7 @@
"slug": "one",
"name": "One",
"iteration": 1,
+ "what_next_instructions": "Your fake solution for one has been submitted.\n\nProgrammers generally spend far more time reading code than writing it.\nTo benefit the most from this exercise, find 3 or more submissions that you can\nlearn something from, have questions about, or have suggestions for.\nPost your thoughts and questions in the comments, and start a discussion.\nConsider revising your solution to incorporate what you learn.\n\nYours and others' solutions to this problem:\nhttp://example.org/tracks/fake/exercises/one\n",

This comment has been minimized.

@kytrinyx

kytrinyx Mar 19, 2017

Member

:-)

I'd probably just leave this as the first sentence, to keep the fixture a bit more terse.

@kytrinyx

kytrinyx Mar 19, 2017

Member

:-)

I'd probably just leave this as the first sentence, to keep the fixture a bit more terse.

This comment has been minimized.

@nilbus

nilbus Mar 19, 2017

Member

That would allow the test to be a lot more flexible too, not having to update these three examples every time we modify the wording.

I think modifying Approvals to handle something more complicated than a straight comparison is something to address later though, in favor of implementing changes for the experiment. Maybe next time we do change the wording.

@nilbus

nilbus Mar 19, 2017

Member

That would allow the test to be a lot more flexible too, not having to update these three examples every time we modify the wording.

I think modifying Approvals to handle something more complicated than a straight comparison is something to address later though, in favor of implementing changes for the experiment. Maybe next time we do change the wording.

This comment has been minimized.

@kytrinyx

kytrinyx Mar 19, 2017

Member

Oh, I see what's going on. Sorry—my bad. I didn't realize that this is an approval (generated). For some reason I thought it was a hand-crafted fixture. For approvals I am actually pretty comfortable just re-saving new files when they change.

@kytrinyx

kytrinyx Mar 19, 2017

Member

Oh, I see what's going on. Sorry—my bad. I didn't realize that this is an approval (generated). For some reason I thought it was a hand-crafted fixture. For approvals I am actually pretty comfortable just re-saving new files when they change.

@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Mar 19, 2017

Member

Is there a better place than the route file to define the copy for the what next instructions?

Nah, I think that's fine. I think it makes sense to keep it close to where it's used. If we start having a lot of these sorts of things, we might have some sort of config file somewhere, but I think adding indirection would be overkill here.

Are you happy with the copy?

Yes, it looks good. In the earlier PR that hard-coded it in the CLI you had two versions based on the experiment. Will you be adding the old copy to flip between them?

One last thought: should we record the version of the CLI that someone submits an iteration with? That would let us determine who saw the feature or not.

Member

kytrinyx commented Mar 19, 2017

Is there a better place than the route file to define the copy for the what next instructions?

Nah, I think that's fine. I think it makes sense to keep it close to where it's used. If we start having a lot of these sorts of things, we might have some sort of config file somewhere, but I think adding indirection would be overkill here.

Are you happy with the copy?

Yes, it looks good. In the earlier PR that hard-coded it in the CLI you had two versions based on the experiment. Will you be adding the old copy to flip between them?

One last thought: should we record the version of the CLI that someone submits an iteration with? That would let us determine who saw the feature or not.

@nilbus

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Mar 19, 2017

Member

should we record the version of the CLI that someone submits an iteration with? That would let us determine who saw the feature or not.

Both that, and it could tell us whether or not to show a banner asking the user to upgrade.

I'll open a PR for cli to include the version in at least submission API calls.

Member

nilbus commented Mar 19, 2017

should we record the version of the CLI that someone submits an iteration with? That would let us determine who saw the feature or not.

Both that, and it could tell us whether or not to show a banner asking the user to upgrade.

I'll open a PR for cli to include the version in at least submission API calls.

@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Mar 19, 2017

Member

I'll open a PR for cli to include the version in at least submission API calls.

I think it already sends the value as an HTTP header.

Member

kytrinyx commented Mar 19, 2017

I'll open a PR for cli to include the version in at least submission API calls.

I think it already sends the value as an HTTP header.

@nilbus

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Mar 20, 2017

Member

I'll code for cli and keep in mind later expansion to gui.

Member

nilbus commented Mar 20, 2017

I'll code for cli and keep in mind later expansion to gui.

@nilbus

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Mar 20, 2017

Member

I posted some ideas in #3416. For our banner for this experiment, I'll implement something simpler (in a separate pull request).

Member

nilbus commented Mar 20, 2017

I posted some ideas in #3416. For our banner for this experiment, I'll implement something simpler (in a separate pull request).

@kytrinyx kytrinyx merged commit fb0ee79 into exercism:master Mar 20, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 85.57%
Details
@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Mar 20, 2017

Member

Thanks @nilbus!

Member

kytrinyx commented Mar 20, 2017

Thanks @nilbus!

@nilbus nilbus deleted the nilbus:instructions-in-submission-response branch Mar 22, 2017

@nilbus nilbus referenced this pull request in exercism/discussions Mar 26, 2017

Closed

Studying game design for increasing code review participation #123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment