search

lrstanley / girc

:bomb: girc is a flexible IRC library for Go :ok_hand:
https://pkg.go.dev/github.com/lrstanley/girc
MIT License
138 stars 13 forks source link

Fix pretty printing of nick messages #15

Closed nmeum closed 5 years ago

nmeum commented 6 years ago

The nick the user switched to seems to be encoded using event.Trailing, thus len(event.Params) is always 0 for nick messages. This commit fixes this by removing the length check and using event.Trailing instead of event.Params.

codecov-io commented 6 years ago

Codecov Report

Merging #15 into master will decrease coverage by 0.14%. The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
- Coverage   56.46%   56.31%   -0.15%     
==========================================
  Files          13       13              
  Lines        2313     2319       +6     
==========================================
  Hits         1306     1306              
- Misses        901      907       +6     
  Partials      106      106
Impacted Files Coverage Δ
event.go 67.79% <0%> (-1.56%) :arrow_down:

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 a5d4d6a...dfeefc5. Read the comment docs.

lrstanley commented 6 years ago

For this, I'm not sure every implementation is going to utilize trailing parameters for NICK commands. Some are NICK newname and some are NICK :newname. I need to move away from the use of Event.Trailing, and merge it with Event.Params and an Event.ParamLast() or maybe Event.HasTrailing (boolean). This isn't how most other IRC libs handle it, but this would technically be the correct way (and would fix this type of problem (https://modern.ircdocs.horse/):

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.

Will probably hold off on merging this unless you want to update it to check for both for now, kinda like I do here:

https://github.com/lrstanley/girc/blob/a5d4d6aadaa1cbf635cbc213818c39529a1c3ce2/builtin.go#L329-L333

nmeum commented 5 years ago

I need to move away from the use of Event.Trailing …

I agree that this would be the proper solution. However, it doesn't seem like this is going to happen anytime soon thus it would be nice to have a workaround in place for the meantime. Therefore, I implemented implemented the check for both options as in builtin.go.