Skip to content

Conversation

@zbynek
Copy link
Collaborator

@zbynek zbynek commented Jun 18, 2024

Adds emulation for java.text.Normalizer using the native normalize method on String.

@zbynek zbynek marked this pull request as draft June 18, 2024 22:33
@zbynek zbynek force-pushed the normalizer branch 2 times, most recently from b62a424 to 09928d1 Compare June 18, 2024 23:24
Copy link
Member

@niloc132 niloc132 left a comment

Choose a reason for hiding this comment

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

Checkstyle apparently crashed https://github.com/gwtproject/gwt/actions/runs/9573687321/job/26395563622?pr=9970#step:6:11634. Give it a shot locally, see if you can see why? I can give it it a shot if it isn't clear after you try - the newer checkstyle does seem to be a bit slower and prone to issues...

Aside from the extra newlines, only thing at a glance that looks off to me is the 4-space indentation, rather than the project standard of 2-space.

import jsinterop.annotations.JsType;

/**
* Emulation of {@code java.text.Normalizer}.
Copy link
Member

Choose a reason for hiding this comment

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

Note that this is circular - maybe a link to javadoc instead? e.g. https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/text/Normalizer.html

import java.text.Normalizer;
import java.text.Normalizer.Form;

public class NormalizerTest extends EmulTestBase {
Copy link
Member

Choose a reason for hiding this comment

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

This won't run if not added to a suite.

zbynek and others added 3 commits June 19, 2024 05:59
Co-authored-by: Colin Alworth <colin@vertispan.com>
Co-authored-by: Colin Alworth <colin@vertispan.com>
Co-authored-by: Colin Alworth <colin@vertispan.com>
@zbynek zbynek marked this pull request as ready for review June 19, 2024 12:52
Copy link
Member

@niloc132 niloc132 left a comment

Choose a reason for hiding this comment

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

Checkstyle is happy, tests pass locally.

Typically we're a little conservative with adding new JRE emulation, but since this is so close to what the browser itself offers, it seems to make good sense to land this.

It should be noted that elemental2's JsString also supports these (but we don't want GWT's JRE to depend on elemental2 at all).

Co-authored-by: Colin Alworth <colin@vertispan.com>
Copy link
Member

@niloc132 niloc132 left a comment

Choose a reason for hiding this comment

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

Approved by me at least, will give it another day or so for anyone else to comment.

@niloc132 niloc132 merged commit aabb575 into gwtproject:main Jul 8, 2024
@zbynek zbynek deleted the normalizer branch July 8, 2024 18:40
@niloc132 niloc132 added this to the 2.12 milestone Aug 9, 2024
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.

2 participants