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

Split long replies if requested with -chunk #45

Merged
merged 3 commits into from Nov 7, 2018
Merged

Conversation

insomniacslk
Copy link
Owner

Fixes #38

@insomniacslk insomniacslk added the enhancement New feature or request label Nov 6, 2018
@insomniacslk insomniacslk self-assigned this Nov 6, 2018
@insomniacslk
Copy link
Owner Author

@Syloq can you try this patch? More testing is coming

@codecov
Copy link

codecov bot commented Nov 6, 2018

Codecov Report

Merging #45 into master will increase coverage by 3.23%.
The diff coverage is 48.78%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #45      +/-   ##
=========================================
+ Coverage    2.89%   6.13%   +3.23%     
=========================================
  Files           6       7       +1     
  Lines         552     587      +35     
=========================================
+ Hits           16      36      +20     
- Misses        536     551      +15
Impacted Files Coverage Δ
irc_context.go 0% <0%> (ø) ⬆️
irc_server.go 0% <0%> (ø) ⬆️
main.go 0% <0%> (ø) ⬆️
server.go 0% <0%> (ø) ⬆️
wordwrap.go 100% <100%> (ø)

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 4a6773b...4892bb9. Read the comment docs.

@Syloq
Copy link

Syloq commented Nov 7, 2018

I'm getting the same result as before the patch. I need to really debug the ZNC connection and see what is going on exactly. I see that you send the numeric 353 but ZNC does not like it and replies with a :"Received a too long line from the IRC server!" to the IRC client. I can't be exactly sure what is triggering the error. I'll look at it more tonight.

@insomniacslk
Copy link
Owner Author

Have you run it with -chunk 512 ?

@insomniacslk
Copy link
Owner Author

also CC @chex3, you have a lot of experience with large Slack teams :)

@Syloq
Copy link

Syloq commented Nov 7, 2018

Have you run it with -chunk 512 ?

Ugh, sorry I misunderstood what you did there. Yes, that appears to work as expected.

@insomniacslk
Copy link
Owner Author

it seems to fix some issues that I have on another large Slack team that I use for testing - now I don't have random surprises with the nick list anymore. I think it may be good to have this as default - maybe 1024 like most clients can deal with? Or 512 to make everyone happy at the cost of over-chunking?

@Syloq
Copy link

Syloq commented Nov 7, 2018

I'd go with 512 as the default and allow people to change it via the switch on the command line if they see fit. Seems like the best of both worlds.

@insomniacslk
Copy link
Owner Author

Alright, be it 512. Testing and merging soon

@insomniacslk insomniacslk merged commit e1eda43 into master Nov 7, 2018
@insomniacslk insomniacslk deleted the split_replies branch November 7, 2018 18:16
@Syloq
Copy link

Syloq commented Nov 9, 2018

So, I'm not sure if this is related. I have the chunking working properly now, but after running the irc-slack app for over a few hours with little to no activity I am no longer getting messages from slack. I see people sending them privately or in a channel via the slack web interface, but it isn't showing up in the client, nor in the log of the app itself.

I know this isn't overly helpful information. But I'm not sure if its related to this change or not.

@insomniacslk
Copy link
Owner Author

@Syloq do you see any relevant or weird message in the status bar? Or in the irc-slack logs?

@Syloq
Copy link

Syloq commented Nov 14, 2018

Sorry for the late reply. I think this was related to the other PR that came after this one. I was able to pull from master and try to replicate, but it hasn't had the same issue since. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants