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

chore(text): release [email protected] #5209

Merged
merged 3 commits into from
Jul 17, 2024
Merged

chore(text): release [email protected] #5209

merged 3 commits into from
Jul 17, 2024

Conversation

iuioiua
Copy link
Contributor

@iuioiua iuioiua commented Jul 1, 2024

To be merged July 11.

The latest API docs: https://jsr.io/@std/[email protected]

Closes #4999

@iuioiua iuioiua requested a review from kt3k as a code owner July 1, 2024 03:34
@github-actions github-actions bot added the text label Jul 1, 2024
Copy link

codecov bot commented Jul 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.35%. Comparing base (317a0d8) to head (9a8abf6).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5209   +/-   ##
=======================================
  Coverage   96.35%   96.35%           
=======================================
  Files         463      463           
  Lines       37728    37728           
  Branches     5571     5571           
=======================================
  Hits        36354    36354           
  Misses       1332     1332           
  Partials       42       42           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kt3k kt3k requested a review from magurotuna July 10, 2024 05:01
@magurotuna
Copy link
Member

closestString

https://github.com/denoland/deno_std/blob/22d3bda488f145b725fc1eaeee16922a97d88add/text/closest_string.ts#L27-L31

This note sounds concerning to me when it comes to stabilization. I'd suggest that we pin the algorithm that this function internally uses to calculate similarity, or make it configurable by an optional parameter and default to Levenshtein if none is specified by a user.

compareSimilarity

https://github.com/denoland/deno_std/blob/22d3bda488f145b725fc1eaeee16922a97d88add/text/compare_similarity.ts#L41-L44

ditto

wordSimilaritySOrt

https://github.com/denoland/deno_std/blob/22d3bda488f145b725fc1eaeee16922a97d88add/text/word_similarity_sort.ts#L48

ditto

toCamelCase

This is defined in case.ts not to_camel_case.ts, so we can't import it with import { toCamelCase } from "jsr:@std/text/to-camel-case" but instead should do like jsr:@std/text/case. This would be surprising to users because usually one public function has one module that allows us to import only that function but toCamelCase doesn't follow this convention.

The same goes for toKebabCase, toPascalCase, and toSnakeCase

@kt3k
Copy link
Member

kt3k commented Jul 12, 2024

This is defined in case.ts not to_camel_case.ts, so we can't import it with import { toCamelCase } from "jsr:@std/text/to-camel-case"

Maybe it's better to align to that rule in this case. @iuioiua Do you have any opinions?

BTW there are multiple places where we break that rule. The examples are:

  • html/entities
  • http/*
  • testing/*
  • fmt/*
  • encoding/*
  • uuid/*

@kt3k
Copy link
Member

kt3k commented Jul 12, 2024

make it configurable by an optional parameter and default to Levenshtein if none is specified by a user.

This sounds reasonable to me. Thanks for the suggestion!

@iuioiua
Copy link
Contributor Author

iuioiua commented Jul 12, 2024

All good suggestions. Happy to proceed with them.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@magurotuna magurotuna left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@iuioiua iuioiua merged commit c699899 into main Jul 17, 2024
16 checks passed
@iuioiua iuioiua deleted the text-1 branch July 17, 2024 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stabilize @std/text
3 participants