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

Several updates/fixes to example script #29

Merged
merged 1 commit into from Jun 5, 2017

Conversation

Projects
None yet
2 participants
@kyzn
Contributor

kyzn commented May 29, 2017

Thanks for maintaining this module! It was my CPAN pull request challenge assignment for the month, and I realized example script wasn't working properly. So I went ahead and figured out why, and rewrote a few methods to fix it.

Let me know if you want me to update anything. Thanks!

Several updates/fixes to example script
- Replace Mojo::Redis (deprecated) with Mojo::Redis2
- Update all $redis usage according to their new API
- Rename /:id.json as /id/json to prevent /1234 being catched as "id.json"
- Use Mojo::JSON's decode/encode_json (as opposed to Mojo::JSON->new)
- Update readme to make it even more obvious
- Add a 'not_found' templated that can be rendered
- Remove trailing spaces

@kyzn kyzn referenced this pull request May 29, 2017

Merged

Subscribe expects an arrayref #22

@@ -60,40 +56,47 @@ get '/server/:server' => sub {
}
else
{
$self->render_not_found;
$self->render('not_found');

This comment has been minimized.

@plicease

plicease May 29, 2017

Owner

This should be $self->reply->not_found so that it returns a 404. See https://metacpan.org/pod/Mojolicious::Plugin::DefaultHelpers#reply->not_found

@plicease

plicease May 29, 2017

Owner

This should be $self->reply->not_found so that it returns a 404. See https://metacpan.org/pod/Mojolicious::Plugin::DefaultHelpers#reply->not_found

@plicease

This comment has been minimized.

Show comment
Hide comment
@plicease

plicease May 29, 2017

Owner

Thanks! I wrote this example a while ago and Mojolicious has obviously altered its API a bit since then. I had one comment so far and will review this more closely when I get back home this week, but this looks very good!

Owner

plicease commented May 29, 2017

Thanks! I wrote this example a while ago and Mojolicious has obviously altered its API a bit since then. I had one comment so far and will review this more closely when I get back home this week, but this looks very good!

@plicease plicease merged commit 4172344 into plicease:master Jun 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@plicease

This comment has been minimized.

Show comment
Hide comment
@plicease

plicease Jun 5, 2017

Owner

I made one minor tweak based on my comment earlier and merged. Thanks again.

Owner

plicease commented Jun 5, 2017

I made one minor tweak based on my comment earlier and merged. Thanks again.

@kyzn

This comment has been minimized.

Show comment
Hide comment
@kyzn

kyzn Jun 6, 2017

Contributor

Thanks! I guess in that case we wouldn't need last two lines too:

@@ not_found.html.ep
Not found
Contributor

kyzn commented Jun 6, 2017

Thanks! I guess in that case we wouldn't need last two lines too:

@@ not_found.html.ep
Not found
@plicease

This comment has been minimized.

Show comment
Hide comment
@plicease

plicease Jun 6, 2017

Owner

Yeah I debated that. It actually uses the not_found template when you reply with not found.

Owner

plicease commented Jun 6, 2017

Yeah I debated that. It actually uses the not_found template when you reply with not found.

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