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

Rewrite Error message Address in use #485 #487

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@jluis

jluis commented Apr 7, 2016

"Can't create listen socket: Address already in use" is rewriten as
"Can't create listen socket on port $port: Address already in use"

Signed-off-by: Jose Luis Perez Diez jluis@escomposlinux.org

@preaction

This comment has been minimized.

Show comment
Hide comment
@preaction

preaction Apr 7, 2016

Owner

Thanks! You'll probably want to use $daemon->listen instead to get the address that can't be listened to (see http://mojolicious.org/perldoc/Mojo/Server/Daemon#listen). That way if Mojolicious changes how it finds the listen address, we aren't caught trying to keep up.

There needs to be a test written. Starting up one daemon before trying to start a second should be enough to make the failure case happen.

Is this the only error message that could be thrown by $daemon->start? It might be better to use a regex to match $@ before customizing the error message.

Owner

preaction commented Apr 7, 2016

Thanks! You'll probably want to use $daemon->listen instead to get the address that can't be listened to (see http://mojolicious.org/perldoc/Mojo/Server/Daemon#listen). That way if Mojolicious changes how it finds the listen address, we aren't caught trying to keep up.

There needs to be a test written. Starting up one daemon before trying to start a second should be enough to make the failure case happen.

Is this the only error message that could be thrown by $daemon->start? It might be better to use a regex to match $@ before customizing the error message.

@jluis

This comment has been minimized.

Show comment
Hide comment
@jluis

jluis Apr 7, 2016

Thanks for your pointers.

I did the pull request so early, because I had not noticed that pushed commits that reference an issue got a link in the issue thread, to get feedback.

I plan to look at Mojo source to see how and what messages throws when dies as I have noticed that this particular message has parts that depend on $ENV{LANG} .

I will also create a test for it on t/command/daemon.t

I suppose that it is OK to include in the pull request both files and all chain of commits, isn't it?

jluis commented Apr 7, 2016

Thanks for your pointers.

I did the pull request so early, because I had not noticed that pushed commits that reference an issue got a link in the issue thread, to get feedback.

I plan to look at Mojo source to see how and what messages throws when dies as I have noticed that this particular message has parts that depend on $ENV{LANG} .

I will also create a test for it on t/command/daemon.t

I suppose that it is OK to include in the pull request both files and all chain of commits, isn't it?

@preaction

This comment has been minimized.

Show comment
Hide comment
@preaction

preaction Apr 8, 2016

Owner

This seems like it's a single change, so it should likely be a single commit. So you can amend your current commit, and then force-push to update the pull request. Otherwise, I can squash the commits when I merge, either way.

Owner

preaction commented Apr 8, 2016

This seems like it's a single change, so it should likely be a single commit. So you can amend your current commit, and then force-push to update the pull request. Otherwise, I can squash the commits when I merge, either way.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 8, 2016

Coverage Status

Coverage decreased (-0.2%) to 92.633% when pulling 335a5a0 on jluis:cpanpr into 29e2c92 on preaction:master.

coveralls commented Apr 8, 2016

Coverage Status

Coverage decreased (-0.2%) to 92.633% when pulling 335a5a0 on jluis:cpanpr into 29e2c92 on preaction:master.

Rewrite Error message Address in use #485
"Can't create listen socket: Address already in use" is rewriten as
"Can't create listen socket for (http://*:3000): Address already in use"
it changes all "Can't create listen socket:" messages to
"Can't create listen socket for ($listen_URL):"

It has a test in t/command/daemon.t that test the new messge when no port
is specified


Signed-off-by: Jose Luis Perez Diez <jluis@escomposlinux.org>
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 11, 2016

Coverage Status

Coverage increased (+0.03%) to 92.874% when pulling 372257e on jluis:cpanpr into dc7d8e8 on preaction:master.

coveralls commented Apr 11, 2016

Coverage Status

Coverage increased (+0.03%) to 92.874% when pulling 372257e on jluis:cpanpr into dc7d8e8 on preaction:master.

@jluis

This comment has been minimized.

Show comment
Hide comment
@jluis

jluis Apr 12, 2016

I messed the pull request a little as I don't use normally amended commits.
i@preaction I will try to addres other isues now

jluis commented Apr 12, 2016

I messed the pull request a little as I don't use normally amended commits.
i@preaction I will try to addres other isues now

@preaction

This comment has been minimized.

Show comment
Hide comment
@preaction

preaction Apr 14, 2016

Owner

No worries, we can clean up the commits later. Let me know if you've got any questions.

Owner

preaction commented Apr 14, 2016

No worries, we can clean up the commits later. Let me know if you've got any questions.

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