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

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

Merged
merged 2 commits into from Mar 20, 2017

Conversation

nilbus
Copy link

@nilbus 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.

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
Copy link
Author

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:.

@nilbus nilbus force-pushed the instructions-in-submission-response branch from e02f051 to 141d5e9 Compare March 19, 2017 02:47
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
@nilbus nilbus force-pushed the instructions-in-submission-response branch from 141d5e9 to eec01d7 Compare March 19, 2017 02:48
@@ -1,7 +1,7 @@
inherit_from: .rubocop_todo.yml

AllCops:
TargetRubyVersion: 2.2
TargetRubyVersion: 2.3
Copy link
Member

Choose a reason for hiding this comment

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

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",
Copy link
Member

Choose a reason for hiding this comment

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

:-)

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
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.

@nilbus
Copy link
Author

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
Copy link
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.

@nilbus
Copy link
Author

nilbus commented Mar 20, 2017

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

@nilbus
Copy link
Author

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
@kytrinyx
Copy link
Member

Thanks @nilbus!

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.

None yet

2 participants