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

Implementation of Whois feature. #74

Merged
merged 3 commits into from
Dec 11, 2017
Merged

Implementation of Whois feature. #74

merged 3 commits into from
Dec 11, 2017

Conversation

tchoutri
Copy link
Collaborator

@tchoutri tchoutri commented Dec 9, 2017

Summary of the changes

Hi @bitwalker! I have made the following changes in the code base:

  • Implemented the relevant handle_data functions to catch the messages
  • Added the numerics in Irc.Commands, except for @rpl_whoismodes that I have not encountered yet.
  • Added a whois_buffers key in %ClientState to store the intermediate results.
  • Added an %Irc.Whois{} struct to enforce the authorized keys and ensure data integrity and document the available fields.
  • Updated the code to use the non-deprecated versions of String.strip and String.to_char_list.

@coveralls
Copy link

coveralls commented Dec 9, 2017

Coverage Status

Coverage decreased (-1.7%) to 32.353% when pulling 1ff4acf on tchoutri:add-whois into f4c96bb on bitwalker:master.

@coveralls
Copy link

coveralls commented Dec 9, 2017

Coverage Status

Coverage decreased (-1.6%) to 32.426% when pulling d185753 on tchoutri:add-whois into f4c96bb on bitwalker:master.

@shymega
Copy link
Contributor

shymega commented Dec 11, 2017

@tchoutri This looks like a good PR, but I have just a few thoughts on it:

It would be even better with documentation: in the code itself, the examples directory (i.e, showing how to use the WHOIS function), and in the README perhaps, but the examples directory might cover that.. now I think about it.

The code itself looks good, but I'm not sure how to use it. I'll have another play when I get some more time. Good work though, although I'm not the maintainer, I've contributed before, and it certainly looks neater than my contributions 👍

{:noreply, %ClientState{state|whois_buffers: Map.put(state.whois_buffers, nickname, user)}}
end

def handle_data(%IrcMessage{cmd: @rpl_whoiscertfp, args: [_sender, nickname, "has client certificate fingerprint "<> fingerprint]}, state) do
Copy link
Owner

Choose a reason for hiding this comment

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

Are we certain this text is standardized? It seems really fragile to match on it like this (though I'm not sure there is a better approach).

{:noreply, %ClientState{state|whois_buffers: put_in(state.whois_buffers, [nickname, :registered_nick?], true)}}
end

def handle_data(%IrcMessage{cmd: @rpl_whoishelpop, args: [_sender, nickname, "is available for help."]}, state) do
Copy link
Owner

Choose a reason for hiding this comment

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

Same thing about the text matching

end

def handle_data(%IrcMessage{cmd: @rpl_whoisserver, args: [_sender, nickname, server_addr, server_name]}, state) do
new_buffer = state.whois_buffers |> put_in([nickname, :server_name], server_name) |> put_in([nickname, :server_address], server_addr)
Copy link
Owner

Choose a reason for hiding this comment

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

can we change the formatting here to:

new_buffer =
  state.whois_buffers
  |> put_in(...)
  ...

Copy link
Owner

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

My main concern is that we're matching on text messages in handle_data a lot, is that necessary when we have the RPL code to match on?

Otherwise looks good!

@tchoutri
Copy link
Collaborator Author

Yes, in some cases, we will need to have the text to make the distinction between, for instance, RPL_WHOISSADMIN and RPL_NOTIFYACTION (numeric 308), or RPL_WHOISSADMIN and RPL_WHOISHELPER . Look at this up-to-date page about IRC Numerics for more informations.

However, after some reflexion, I suggest we deal with those on a case-by-case basis. :)

@coveralls
Copy link

coveralls commented Dec 11, 2017

Coverage Status

Coverage decreased (-1.6%) to 32.426% when pulling 2fb329a on tchoutri:add-whois into f4c96bb on bitwalker:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.6%) to 32.426% when pulling 868244e on tchoutri:add-whois into a2f3e39 on bitwalker:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.6%) to 32.426% when pulling 868244e on tchoutri:add-whois into a2f3e39 on bitwalker:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.6%) to 32.426% when pulling 868244e on tchoutri:add-whois into a2f3e39 on bitwalker:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.6%) to 32.426% when pulling 868244e on tchoutri:add-whois into a2f3e39 on bitwalker:master.

@bitwalker bitwalker merged commit a7c3b34 into bitwalker:master Dec 11, 2017
@tchoutri tchoutri deleted the add-whois branch December 11, 2017 21:26
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.

4 participants