Skip to content
  • Sponsor stdlib-js/stdlib

  • Notifications You must be signed in to change notification settings
  • Fork 805

feat: add assert/is-well-formed-string #1388

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

Merged
merged 23 commits into from
Feb 27, 2024
Merged

feat: add assert/is-well-formed-string #1388

merged 23 commits into from
Feb 27, 2024

Conversation

gunjjoshi
Copy link
Member

@gunjjoshi gunjjoshi commented Feb 26, 2024

This commit adds the assert/is-well-formed-string module for checking well-formed-strings.

Resolves #1065

Description

Added the is-well-formed-string package. It checks for four things to check if a string is well formed or not:

  1. A low surrogate is present at the beginning.
  2. A high surrogate is present at the last.
  3. No low surrogate after a high surrogate.
  4. No high surrogate before a low surrogate.

What is the purpose of this pull request?

This pull request:

  • Adds a package to check if a string is well-formed or not.

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

Screenshot of tests:

Screenshot 2024-02-26 at 10 53 57

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
This commit adds the string/is-well-formed-string module for checking well-formed-strings.
This commit adds the assert/is-well-formed-string module for checking well-formed-strings.
@gunjjoshi gunjjoshi changed the title Is well formed string package Added is-well-formed-string package Feb 26, 2024
@Planeshifter Planeshifter changed the title Added is-well-formed-string package feat: add assert/is-well-formed-string Feb 26, 2024
@Planeshifter Planeshifter self-requested a review February 26, 2024 16:01
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Looks very good; left some comments that will need to be addressed before merging.

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>

var RE_UTF16_LOW_SURROGATE = /[\uDC00-\uDFFF]/;
var RE_UTF16_HIGH_SURROGATE = /[\uD800-\uDBFF]/;
var i = 0;
Copy link
Member

Choose a reason for hiding this comment

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

We should not initialize the i variable outside the function scope. Just declare var i; in L45 before the loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed that all variables should be declared in the variables section.
But yes, it is outside the scope of the function, and we should avoid using global variables.
I've made the suggested change.

gunjjoshi and others added 18 commits February 27, 2024 07:14
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
…itive.js

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
…es/test.ts

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
…es/test.ts

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
…/index.js

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
…llformed.js

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
…llformed.js

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
…llformed.js

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
…llformed.js

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
…llformed.js

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
…llformed.js

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
…llformed.js

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@gunjjoshi
Copy link
Member Author

@Planeshifter
There is some error in linting the examples. I have written the correct outputs, but it is expecting something else, and thus giving errors. I checked the implementation of the functions, it is indeed correct.

Screenshot 2024-02-27 at 10 36 46

@Planeshifter
Copy link
Member

@gunjjoshi Thanks for checking! I believe the issue is that the instanceof String check doesn't work across realms when there are multiple String constructors. To lint examples and their expected outputs, we spawn a separate process via vm2 that will have it's own String constructor. Here is how we make the object check in the assert/is-string package https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/assert/is-string/lib/object.js more robust to have it work across realms.
Would you mind updating this package accordingly?

@gunjjoshi
Copy link
Member Author

@gunjjoshi Thanks for checking! I believe the issue is that the instanceof String check doesn't work across realms when there are multiple String constructors. To lint examples and their expected outputs, we spawn a separate process via vm2 that will have it's own String constructor. Here is how we make the object check in the assert/is-string package https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/assert/is-string/lib/object.js more robust to have it work across realms. Would you mind updating this package accordingly?

Sure, I'll update this package.

This commit adds the assert/is-well-formed-string module for checking well-formed-strings.
@gunjjoshi
Copy link
Member Author

gunjjoshi commented Feb 27, 2024

Great ! It got passed by the CI pipeline.
So the string object checking was creating the problem.

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
@Planeshifter
Copy link
Member

@gunjjoshi Will merge after CI clears; thanks again for your contribution!

@Planeshifter Planeshifter merged commit 0f3838c into stdlib-js:develop Feb 27, 2024
@gunjjoshi gunjjoshi deleted the is-well-formed-string-package branch February 27, 2024 17:58
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.

[RFC]: add @stdlib/assert/is-well-formed-string
2 participants