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

initial work towards moving away from Event.Trailing (closes #5) #36

Merged
merged 7 commits into from Feb 10, 2019

Conversation

@lrstanley
Copy link
Owner

@lrstanley lrstanley commented Jan 5, 2019

Closes: #35
Closes: #19
See also: #15 (comment)


From: https://modern.ircdocs.horse/

Here are some examples of messages and how the parameters would be represented as JSON lists:

  :irc.example.com CAP * LIST :         ->  ["*", "LIST", ""]

  CAP * LS :multi-prefix sasl           ->  ["*", "LS", "multi-prefix sasl"]

  CAP REQ :sasl message-tags foo        ->  ["REQ", "sasl message-tags foo"]

  :dan!d@localhost PRIVMSG #chan :Hey!  ->  ["#chan", "Hey!"]

  :dan!d@localhost PRIVMSG #chan Hey!   ->  ["#chan", "Hey!"]

As the last two examples show, a trailing parameter (a parameter prefixed with ':') is another regular parameter. Once the ':' is stripped, software MUST just treat it as another param.


Essentially, Event.Trailing is now just an additional entry in the Event.Params slice.

I'm still opting to have the concept of Trailing as a semantic wrapper (and has nothing to do with the rfc's use of "trailing"). Event.Trailing(), which now just returns the last parameter/argument (from Event.Params directly), which will help as it is much more frequent to get the last element, than any other element (and Event.Params[len(Event.Params)-1] is very verbose). The other benefit is it defaults to an empty string. Maybe I'll rename it to Last?

This probably has bugs in it. Tests run fine, did some initial connections to servers, but I'd like some contributors to test this out before I merge into master:

Also, @qaisjp, this has that spelling correction.

@codecov-io
Copy link

@codecov-io codecov-io commented Jan 5, 2019

Codecov Report

Merging #36 into master will increase coverage by 0.13%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
+ Coverage   56.04%   56.18%   +0.13%     
==========================================
  Files          13       13              
  Lines        2339     2321      -18     
==========================================
- Hits         1311     1304       -7     
+ Misses        921      912       -9     
+ Partials      107      105       -2
Impacted Files Coverage Δ
format.go 97.16% <ø> (ø) ⬆️
commands.go 0% <0%> (ø) ⬆️
cap.go 22.77% <0%> (ø) ⬆️
cap_sasl.go 0% <0%> (ø) ⬆️
client.go 70.27% <0%> (ø) ⬆️
ctcp.go 69.49% <100%> (ø) ⬆️
event.go 69.43% <39.53%> (+2.16%) ⬆️
builtin.go 60.67% <53.57%> (-0.14%) ⬇️
conn.go 58.53% <83.33%> (-0.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1e59a0...057fbbb. Read the comment docs.

Loading

@nmeum
Copy link
Contributor

@nmeum nmeum commented Jan 5, 2019

First of all: Thanks for this patch, this is really useful to me.

I did some quick tests and the first thing I noticed is that /me messages are currently formated incorrectly bei Event.Pretty(). The code for formating them in event.go looks as follows:

if ctcp.Command == CTCP_ACTION {
    return fmt.Sprintf("[%s] **%s** %s", strings.Join(e.Params, ","), ctcp.Source.Name, ctcp.Text), true
}

and should prorably be changed to:

if ctcp.Command == CTCP_ACTION {
    return fmt.Sprintf("[%s] **%s** %s", strings.Join(e.Params[0:len(e.Params)-1], ","), ctcp.Source.Name, ctcp.Text), true
}

Otherwise they are formated as [#channel,ACTION foo] **user** foo instead of [#channel] **user** foo.

Might also be nice to add a helper method for getting all parameters except the trailing one (e.g. e.Params[0:len(e.Params)-1]) since this idiom is used in quite a few places now, but that's up to you.

Loading

@lrstanley
Copy link
Owner Author

@lrstanley lrstanley commented Jan 5, 2019

@nmeum -- Latest commit should have fixes for that and TOPIC's.

I did decide to rename Event.Trailing() to Event.Last(), as Trailing() may mislead (since it's just the last param, not the trailing param necessarily).

I am not sure I want to add a helper to grab all but the last, because it's both not used too frequently (I think this is the only occurrence actually), but it's also not beneficial without also keeping track of if the last item was a trailing item (which means adding a field in Event that says it had a trailing argument).

Loading

builtin.go Outdated Show resolved Hide resolved
Loading
builtin.go Outdated Show resolved Hide resolved
Loading
event.go Show resolved Hide resolved
Loading
nmeum added a commit to nmeum/hii that referenced this issue Jan 5, 2019
This makes a few code parts simpler and also fixes others. For instance,
in the handleKick function we didn't check for Event.Trailing at all.
Meaning a `KICK :<name>` instead of `KICK <name>` message would not have
been parsed properly.

See also: lrstanley/girc#36
@lrstanley
Copy link
Owner Author

@lrstanley lrstanley commented Jan 24, 2019

@42wim have you had a chance to test this?

Loading

@42wim
Copy link
Contributor

@42wim 42wim commented Jan 24, 2019

@lrstanley sorry, this issue was off my radar :(
I've just tested the patch with matterbridge and it didn't found any issues yet. 👍

Loading

@lrstanley lrstanley merged commit 51b8e09 into master Feb 10, 2019
2 of 3 checks passed
Loading
@lrstanley lrstanley deleted the fix35-event-params branch Feb 10, 2019
nmeum added a commit to nmeum/hii that referenced this issue Feb 13, 2019
This makes a few code parts simpler and also fixes others. For instance,
in the handleKick function we didn't check for Event.Trailing at all.
Meaning a `KICK :<name>` instead of `KICK <name>` message would not have
been parsed properly.

See also: lrstanley/girc#36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants