search

lrstanley / girc

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

Don't store RFC 1459 compliant nick in User struct #23

Closed nmeum closed 5 years ago

nmeum commented 5 years ago

The problem with performing ToRFC1459() on the nick before storing it in the User struct is that (among other things) capitalization is lost. So function like client.UserList always return all nicks in lowercase.

I am personally using the values return by channel.UserList for tab completions and to me it is kind of annoying that the capitalization of nicks as shown by event.Pretty() differs from the one suggested by the tab completions I have implemented.

I would suggest that nicks are not stored in an RFC 1459 compliant format since any nick can be made RFC 1459 compliant using the ToRFC1459 function on demand but converting it back to its original format isn't possible.

lrstanley commented 5 years ago

The problem with this, is it will cause girc to have duplicates on many ircd servers. Some servers will return capital letters for some things, and not for others, in addition to many do the conversion for curly braces and similar.

The keys should be standardized IMO -- the spec also says that conversion should be used when doing any comparisons. Though, we could store the original nick in the nick field (and leave the key as converted), I don't have time right now to see if that's already in place.

Regardless, you should convert the nick before comparing. I will leave open to see if there is something I can do to support both.

nmeum commented 5 years ago

The keys should be standardized IMO […]

Totally, agree. I also believe that it is a good idea to use RFC 1459 compliant nicks as map keys.

Though, we could store the original nick in the nick field […]

That's exactly what I am proposing.

I don't have time right now to see if that's already in place.

According to the documentation this isn't the case. The documentation states the following:

// Nick is the users current nickname. rfc1459 compliant.
lrstanley commented 5 years ago

Ran through quickly and added an additional ID field for the Source type. Tried making it's use solely related to things that would be compared.

Pushed it to a different branch if you want to test it out -- I don't have the time at the moment:

https://github.com/lrstanley/girc/tree/fix23-source-id

Hopefully within the next few weeks I will be able to dedicate a bit of time to tackle some of the cleanup, along with some other large plans that will break up girc more logically.

nmeum commented 5 years ago

Thanks for the fix. I didn't take a closer look at it yet.

I was however wondering if it would be more efficient (from a memory usage perspective) to store an unsanitized nick in the Nick member of the source struct and simply convert it to the RFC1459 format on demand (e.g. when performing a nick comparison). Instead of introducing an additional struct member.

nmeum commented 5 years ago

Should we also consider changing the channel.UserList contents? It currently contains RFC1459 compliant nicks other functions such as client.UserList() should return non-RFC1459 compliant nicks now though. Which is somewhat inconsistent.