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

Implement function to prune string keeping HTML closing tags #1870

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

aliksend
Copy link
Contributor

@aliksend aliksend commented Dec 5, 2024

Fixes #1587

@aliksend aliksend requested a review from umputun as a code owner December 5, 2024 22:35
Copy link
Collaborator

@paskal paskal left a comment

Choose a reason for hiding this comment

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

Looks good! Could you please move the prune function to a different file? It's not telegram-specific and now increases complexity of telegram file without necessity.

@aliksend aliksend force-pushed the aliksend/reduce-number-of-symbols-in-tg-message branch from 0585517 to c16c2bf Compare December 17, 2024 04:59
@aliksend
Copy link
Contributor Author

Sure. Sorry for the long wait

Moved function to separated file and rebased my branch onto current master

paskal
paskal previously approved these changes Dec 17, 2024
"golang.org/x/net/html"
)

const commentTextLengthLimit = 100
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the only consonant to move to telegram.go, otherwise LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, my bad. moved back to telegram.go

@aliksend
Copy link
Contributor Author

I don't know why build fails on CI/CD. Locally I can do both go build -ldflags "-X main.revision=0.0.0 -s -w" from backend/_example/memory_store directory and docker buildx build --build-arg SKIP_BACKEND_TEST=true --build-arg SKIP_FRONTEND_TEST=true -f backend/_example/memory_store/Dockerfile . from root directory. Can you help me with it?

@umputun
Copy link
Owner

umputun commented Dec 18, 2024

I don't know why build fails on CI/CD. Locally I can do both go build -ldflags "-X main.revision=0.0.0 -s -w" from backend/_example/memory_store directory and docker buildx build --build-arg SKIP_BACKEND_TEST=true --build-arg SKIP_FRONTEND_TEST=true -f backend/_example/memory_store/Dockerfile . from root directory. Can you help me with it?

fixed, passed

paskal
paskal previously approved these changes Dec 18, 2024
Introduce comprehensive test cases for stringArr methods (Push, Pop, Unshift, Shift, String) to ensure correct behavior and state management. Additionally, add tests for HTML pruning functions (pruneHTML, pruneStringToWord) to validate handling of length constraints and formatting scenarios.
@umputun
Copy link
Owner

umputun commented Dec 18, 2024

I have generated some unit tests, as the one it had didn't seem too convincing to me.

@umputun
Copy link
Owner

umputun commented Dec 18, 2024

pls note - the test is generated to match the code behavoir, however some of them look strange to me. for example this one

		{
 			name:      "text exceeding limit",
 			html:      "<p>Hello world, this is a long text</p>",
 			maxLength: 15,
 			expected:  "<p>...</p>",
 		},

I have not checked the code closely but the common sense expectation would be <p>Hello...</p> in this case.

@aliksend can you take a look pls and adjust tests/code if needed?

@aliksend
Copy link
Contributor Author

Current implementation will generate string "<p>Hello ...</p>" which length is 16, exceeding the limit. But it make sense to remove space in cases like this.
Also I'll fix another tests to make behavior better.

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.

Reduce number of symbols in Telegram messages
3 participants