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

Update scheduled.js #9

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

unbeschwert
Copy link

Added changes to display link of the submissions when they exists.

Added changes to display link of the submissions when they exists.
src/scheduled.js Outdated
event.reply(`${item.title} [${item.categories.join(' ')}] (${username}) ${item.guid}`);
}
else {
event.reply(`${item.title} {${item.link}} [${item.categories.join(' ')}] (${username}) ${item.guid}`);
Copy link
Member

Choose a reason for hiding this comment

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

Can we check the length of the message before we send it? I feel like adding the URL can create a message that's too long for IRC.

Copy link
Author

@unbeschwert unbeschwert Jun 3, 2021

Choose a reason for hiding this comment

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

Hi @hmadison ,

Is the check really necessary ? Recently I got a list of all URLs posted to lobsters from @pushcx and I created a histogram of these URL lengths which you can find below. Most of the URLs lengths are in range of 30 to 80 characters which seems to be fine IMHO.

Figure_1

Copy link
Member

Choose a reason for hiding this comment

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

Pulling the maximum lengths from the schema.rb, we have an upper bound of 707 characters. IRC's max length is 510.

Copy link
Author

@unbeschwert unbeschwert Jun 3, 2021

Choose a reason for hiding this comment

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

Ah okay yes. In that case we need to add checks for length. I'll make the necessary changes. Please do make sure to run some tests on them since I am not familiar with javascript

Edit: @hmadison I just added a new commit that checks if length of reply is greater than 510. I hope this is correct. If it needs some change, please let me know :)

One other idea I had in mind was to use some sort of URL shortnening service. Although they all required paid plans and seemed a bit too much. If its interests you, I can also look it up :)

Copy link

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@winks git.io is for GitHub controlled properties.

Added check for length of the reply. Reply string should not be > 510 characters according to IRC spec.
@hmadison
Copy link
Member

hmadison commented Dec 3, 2021

Hey @unbeschwert, would it be possible to get the test added we discussed on IRC so I can merge this?

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.

3 participants