Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Length is not correctly tracked #153

Open
bnb opened this issue Apr 14, 2021 · 9 comments
Open

Length is not correctly tracked #153

bnb opened this issue Apr 14, 2021 · 9 comments
Labels
bug Something isn't working

Comments

@bnb
Copy link

bnb commented Apr 14, 2021

What happened?

twitter-together failed on a tweet that should have worked, specifically due to line length.

What did you expect to happen?

twitter-together should have succeeded.

What the problem might be

I am fairly confident that this has to do with Twitter's link parsing. Testing out https://github.com/nodejs/build as an example, the .length of that as a string is 31 but Twitter only counts it as 23 chars (note that the space before the link is taking up one char):
image
image

wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww https://github.com/nodejs/build

Given that all characters (ws and the sapce) here count as 280 characters, the actual count is 314 but the Twitter count is 303.

This occurs because under the hood, Twitter creates a t.co short link for every link that is - in total - 23 characters. It doesn't count additional characters in a URL toward the total character count, since they're only storing 23 in total. This will happen for each link, including photos.

@bnb bnb added the bug Something isn't working label Apr 14, 2021
@bnb
Copy link
Author

bnb commented Apr 14, 2021

Here is a specific Action run where this failed despite having a valid tweet length:

https://github.com/nodejs/tweet/pull/26/checks?check_run_id=2347151803

Screenshot from Twitter:
image

This might also indicate something about how the Action is counting emoji vs. how Twitter is counting emoji.

@gr2m
Copy link
Contributor

gr2m commented Apr 14, 2021

We use https://github.com/twitter/twitter-text, it's most likely a problem with that library.

I'm not sure when I'll find the time to look into this I'm afraid, if anyone could investigate and send a PR, I'd be happy to review

@hlwjia
Copy link

hlwjia commented Jan 16, 2022

@gr2m @bnb

From what I just tested with twitter-text, it appears that twitter-text (at least the @3.1.0) is working properly.

Here is my test:

let maxLengthTextForOneLink = 'w'.repeat(280 - 23 - 1)

280 is the max length
23 is the fixed length of shortened URL
1 is the space separating the URL from tweet content (text)

let oneSuperLongLink = 'https://github.com/this-is-obviously-longer-than-23'

A link that's obviously longer than 23 chars

console.log(parseTweet(oneSuperLongLink))

{
  weightedLength: 23,
  valid: true,
  permillage: 82,
  validRangeStart: 0,
  validRangeEnd: 50,
  displayRangeStart: 0,
  displayRangeEnd: 50
}

the weighted length is, as expected, 23

console.log(parseTweet(maxLengthTextForOneLink + ' ' + oneSuperLongLink))

{
  weightedLength: 280,
  valid: true,
  permillage: 1000,
  validRangeStart: 0,
  validRangeEnd: 307,
  displayRangeStart: 0,
  displayRangeEnd: 307
}

The valid property is true


conclusion:

It's probably not the twitter-text library.

@gr2m
Copy link
Contributor

gr2m commented Jan 16, 2022

Thank you for testing John!

@hlwjia
Copy link

hlwjia commented Jan 17, 2022

Upon a bit more digging, I think it has something to do with the whole newline at end of file issue.

I notice it from this PR https://github.com/nodejs/tweet/pull/26/files:

image

Somehow, this "No newline at end of file" hint has been included in the preview:

image

Which I'm highly confident that the added extra length to the original file is the cause of parseTweet(text).valid = false


Here is another PR whose tweet file has newline at end of file:

https://github.com/nodejs/tweet/pull/57/files

and its preview looks good.

A quick fix for @bnb would be to include newline at end of tweet files then.

Reference: https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline


Interestingly, even though the newline thingy is included in the preview checks, I don't think it ever gets tweeted out as part of the sent tweet.

@gr2m I see you use the the same parseTweetFileContent for both pull request and push, not sure why the different behavior.

I think maybe, just maybe, somewhere around here:
https://github.com/gr2m/twitter-together/blob/4fabe65750bfc7eda4b15b5fe9d9e5f52652b572/lib/pull-request/get-new-tweets.js#L47


Btw, great work on this project @gr2m! I had the same thought of making something like twitter-together, and found you've already done an amazing job!

@IstoraMandiri
Copy link
Contributor

IstoraMandiri commented Oct 4, 2022

I believe this should be fixed in the next release.

Here's a test preview running the fix using the tweet at the top of this issue. It's validating as expected now. https://github.com/IstoraMandiri/twitter-together-testing/runs/8690276799

@MattIPv4
Copy link
Member

MattIPv4 commented Oct 4, 2022

This fix should be contained within 2.1.3 that has been shipped :)

@IstoraMandiri
Copy link
Contributor

IstoraMandiri commented Oct 4, 2022

It should be, but it's not automatically building yet (#205), so using the 2.1.3 release still uses the 2.1.1 dist.

https://github.com/IstoraMandiri/twitter-together-testing/actions/runs/3178717079/jobs/5180494130#step:3:1

@MattIPv4
Copy link
Member

MattIPv4 commented Oct 4, 2022

Ah yep, forgot the release doesn't build :(

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants