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

Remove guava in wikitext #562

Closed

Conversation

stephan-herrmann
Copy link

  • address core bundle org.eclipse.mylyn.wikitext

#521

+ org.eclipse.mylyn.wikitext

eclipse-mylyn#521
@stephan-herrmann
Copy link
Author

Various strategies applied:

  • use basic API like Character.isWhitespace() :)
  • use streams
  • implement minimal replacements
    • XML-escaping (class XML) is inspired from utils we have in o.e.resources as well as jdt.
    • only in GfmIdGenerationStrategy.generateId() a little algo was implemented from scratch
  • for the set of MarkupLanguage I felt that filtering duplicates should not be necessary if identical language instances are detected using equals() and hashCode().
    • this required to adjust one test to force duplicates into a set: same name but different classes

@stephan-herrmann
Copy link
Author

My goal is to find out if eclipse-jdt/eclipse.jdt.ui#1472 could be implemented with wikitext (markdown) and without guava. For that task I don't expect to need any of the *.ui bundles, as rendering to HTML should be fully sufficient for JDT. In particular no markdown editing is planned.

@merks
Copy link
Contributor

merks commented Jun 24, 2024

This looks promising.

@gnl42
Copy link
Contributor

gnl42 commented Jun 24, 2024

How will this work live with #521?

@merks
Copy link
Contributor

merks commented Jun 24, 2024

How will this work live with #521?

If I understand correctly, it provides direct implementations that address #521 (comment) for the non-ui parts.

@stephan-herrmann
Copy link
Author

I admit I found #530 only after submitting this PR :)

So the current PR is much narrower in scope, but succeeds to fully eliminate guava dependency from the core plugin without adding any other dependency.

I'm open to discussions to find the best of all worlds. I'm driven by the fact that JDT needs markdown rendering by September the latest.

@ruspl-afed
Copy link
Contributor

@stephan-herrmann thank you for your PR! Perhaps it was my fault to not mention an existing #530 explicitly during our discussion

I would prefer to have #530 merged first, since it was prepared earlier in the scope of #521 and using the same approach as we already have in other parts of Mylyn codebase, and then discuss your change on top of it to reduce the number of required 3rd party libraries. I hope this is fine with you.

How will this work live with #521?

@gnl42 could you please resolve conflicts for #530 so we can merge it and proceed with further discussion?

@stephan-herrmann
Copy link
Author

I would prefer to have #530 merged first, since it was prepared earlier in the scope of #521 and using the same approach as we already have in other parts of Mylyn codebase, and then discuss your change on top of it to reduce the number of required 3rd party libraries. I hope this is fine with you.

Sure, I certainly didn't mean to jump the queue :)

How will this work live with #521?

@gnl42 could you please resolve conflicts for #530 so we can merge it and proceed with further discussion?

If in doubt, some of the more involved changes, that might require a discussion, could perhaps be postponed for later?

@gnl42
Copy link
Contributor

gnl42 commented Jun 30, 2024

#530 has been replaced by #565

@gnl42
Copy link
Contributor

gnl42 commented Jul 1, 2024

@stephan-herrmann Can you recreate your changes against the current main?

@stephan-herrmann
Copy link
Author

@stephan-herrmann Can you recreate your changes against the current main?

See new PR #568

@stephan-herrmann
Copy link
Author

Hopefully #568 will take this to completion.

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