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

Shorten JSON sent to HTTP server #228

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

Conversation

real-or-random
Copy link

This is a first step towards solving #215.

The second commit merges consecutive parts of the same type (TEXT and MARKUP) together. This is what gives most savings in the JSON output. I believe merging TEXTs is fine, but I wonder if merging MARKUPs is too aggressive. When you have a match in a long MARKUP part, possibly stretching over multiple lines, then all the lines are "marked". I doubt that this is wrong, but it's certainly a change from the current behavior. @valentjn What do you think?

@NeckBeardPrince
Copy link

Can we get some 👀 on this PR, please?

Before this commit, "This is a test." is parsed into
  [TEXT("This"), MARKUP(" "), TEXT("is"), MARKUP(" "), TEXT("test.")],
which wastes a lot of characters when converted to JSON.

After this commit, it's parsed into the more natural form
  [TEXT("This is a test.")].
@real-or-random real-or-random force-pushed the 202302-optimize-annotated-text-builder branch from 471b6bd to 8ad7945 Compare May 20, 2023 08:27
@real-or-random
Copy link
Author

rebased on develop

@Lattay
Copy link

Lattay commented Jun 18, 2023

Thanks for the work @real-or-random, I hope we can get a review of this soon. It appears that @valentjn is not very active around here these days (not blaming them, life happens and open-source projects of this scale take a lot of energy).

Although this should avoid the limits for most cases, there will still be a need for splitting requests for very long documents.
Having reviewed the related code, do you see a place where request splitting would be possible?

@real-or-random
Copy link
Author

Having reviewed the related code, do you see a place where request splitting would be possible?

As this concerns only requests sent to external servers, I'd suggest trying in src/main/kotlin/org/bsplines/ltexls/languagetool/LanguageToolHttpInterface.kt first. I think the complex part will be to ensure that we don't hit the rate limits at the external server...

@MarcT0K
Copy link

MarcT0K commented Jan 10, 2024

Thanks for the work! I am particularly interested by this PR and I regret that it is still under review. I wonder whether we should fork the project or provide an external release package. As I have become a regular LTeX user, I don't mind contributing actively to a potential fork but I wonder whether other devs would be interested in giving a hand?

@real-or-random
Copy link
Author

I wonder whether we should fork the project or provide an external release package.

Forking is always easier said than done. I certainly cannot promise to be more active than @valentjn. If you think an external release will help people, that's less work, of course.

@valentjn
It would really help to have some input from you here. How likely is it that you'll work on this again? Do you need help? Are you open to adding another maintainer, assuming @MarcT0K is willing to take it on?

@MarcT0K
Copy link

MarcT0K commented Jan 11, 2024

Forking is always easier said than done.

I agree it won't be easy, especially since I am still discovering the codebase. I am a rather experienced user, but I am totally new to LTeX dev. I don't promise to deliver new features soon. However, a few devs submitted interesting PRs recently so it would be nice to review, test, and merge them. With all the open PRs, we could already prepare a rather nice release.

Indeed, @valentjn if you are still around, please know I am open to giving a hand maintaining your project. I have a lot of stuff to learn about the internals of LTeX, but I really like this software and want to keep it alive.

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.

None yet

4 participants