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

fix: getKebabCase #294 #313

Merged
merged 18 commits into from
Jul 25, 2021
Merged

fix: getKebabCase #294 #313

merged 18 commits into from
Jul 25, 2021

Conversation

fzn0x
Copy link
Contributor

@fzn0x fzn0x commented Jul 22, 2021

Fixes #294 #110

Description

Convert the whitespace and underscore to minus sign without replace the spaces with camel case to fix the issue.

replace(/[\s_]+/g, '-')

Current Tasks

  • Fix: getKebabCase

convert the whitespace and underscore to minus sign without replace it with camel case.
@netlify
Copy link

netlify bot commented Jul 22, 2021

✔️ Deploy Preview for wargabantuwarga ready!

🔨 Explore the source changes: 138c2ff

🔍 Inspect the deploy log: https://app.netlify.com/sites/wargabantuwarga/deploys/60fcbb571730c80008c6372e

😎 Browse the preview: https://deploy-preview-313--wargabantuwarga.netlify.app

@fzn0x
Copy link
Contributor Author

fzn0x commented Jul 22, 2021

PoC
Capture

Copy link
Collaborator

@ekamuktia ekamuktia left a comment

Choose a reason for hiding this comment

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

For the test case mentioned in #294 it's okay

But there are some cases that still not covered, for example:
Text containing special character
"PUTRA SOERADI" Jual Peti Mati: -putra-soeradi-jual-peti-mati
Hoo Hap Hwee (Perkumpulan Budi Abadi): hoo-hap-hwee-perkumpulan-budi-abadi-
Please trim trailing -

Also for text containing ., what is the rule?
Could you confirm the rule and include it on test case? @zainfathoni
For example, with updated function, it'll be:
D.I. Yogyakarta: d.i.-yogyakarta
UD. Bpk. Giyanto Sedia/Jual Peti Jenazah: ud.-bpk.-giyanto-sedia-jual-peti-jenazah
Ambulancesiaga.com: ambulancesiaga.com
Samator, PT. Aneka Gas Industri, Tbk.: samator-pt.-aneka-gas-industri-tbk.
I think the . should be removed, or replaced with -

lib/string-utils.ts Outdated Show resolved Hide resolved
@zainfathoni
Copy link
Member

Please add all the edge cases for into the issue description, Mbak @ekamuktia. We can discuss all the possibilities there.

You should have had access to edit the issue description because I have invited you as a maintainer of this repository. If you aren't able to do it, please check your email to accept the invitation, thanks!

@ekamuktia
Copy link
Collaborator

Test cases updated in #294 description

@fzn0x
Copy link
Contributor Author

fzn0x commented Jul 23, 2021

Test cases updated in #294 description

Capture

just done it with the new test cases, will start commiting ASAP

@fzn0x fzn0x closed this Jul 23, 2021
@fzn0x fzn0x deleted the patch-1 branch July 23, 2021 05:30
@fzn0x fzn0x restored the patch-1 branch July 23, 2021 05:31
@fzn0x fzn0x reopened this Jul 23, 2021
Copy link
Collaborator

@ekamuktia ekamuktia left a comment

Choose a reason for hiding this comment

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

Please fix parts that causing eslint error by adding new line as suggested on the error message

https://github.com/kawalcovid19/wargabantuwarga.com/pull/313/checks?check_run_id=3140847528

fzn0x added 3 commits July 23, 2021 22:30
try to fix eslint , 1st attempt
try to fix eslint error, attempt 2
@zainfathoni
Copy link
Member

zainfathoni commented Jul 24, 2021

Thanks for working on it @fncolon, but we're not merging until we set up a proper unit & integration tests infrastructure for #178, I'm currently working on it at #358. 🙏

@fzn0x
Copy link
Contributor Author

fzn0x commented Jul 24, 2021

Thanks for working on it @fncolon, but we're not merging until we set up a proper unit & integration tests infrastructure for #178, I'm currently working on it at #358. 🙏

Thanks sir Zain, i will keep watching this PR to avoid merge conflicts next time, thanks 🙏

@zainfathoni
Copy link
Member

I've merged #358, so please continue with this PR while updating the test cases to match the expected values from the descriptions. Thanks, @fncolon! 🙏

@fzn0x
Copy link
Contributor Author

fzn0x commented Jul 24, 2021

I've merged #358, so please continue with this PR while updating the test cases to match the expected values from the descriptions. Thanks, @fncolon! 🙏

Hello sir Zain, the expected value from the issue and the test is different, which one should i follow?

@zainfathoni
Copy link
Member

I've merged #358, so please continue with this PR while updating the test cases to match the expected values from the descriptions. Thanks, @fncolon! 🙏

Hello sir Zain, the expected value from the issue and the test is different, which one should i follow?

That expected value was for the old implementation, we need to put those wrong values so that the test doesn't break the main branch build. You should update the test to the actual result of your updated implementation, if the tests are successful, it proves that your regex actually works. 😉

@fzn0x
Copy link
Contributor Author

fzn0x commented Jul 24, 2021

I've merged #358, so please continue with this PR while updating the test cases to match the expected values from the descriptions. Thanks, @fncolon! 🙏

Hello sir Zain, the expected value from the issue and the test is different, which one should i follow?

That expected value was for the old implementation, we need to put those wrong values so that the test doesn't break the main branch build. You should update the test to the actual result of your updated implementation, if the tests are successful, it proves that your regex actually works. 😉

already update the unit test based on current implementation

lib/string-utils.ts Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 25, 2021

Codecov Report

Merging #313 (138c2ff) into main (703c887) will increase coverage by 0.45%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #313      +/-   ##
==========================================
+ Coverage   13.15%   13.60%   +0.45%     
==========================================
  Files          70       67       -3     
  Lines         859      823      -36     
  Branches      229      228       -1     
==========================================
- Hits          113      112       -1     
+ Misses        742      707      -35     
  Partials        4        4              
Impacted Files Coverage Δ
lib/string-utils.ts 36.11% <75.00%> (+4.99%) ⬆️
components/__tests__/report-button.test.tsx
components/__tests__/search-form.test.tsx

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 703c887...138c2ff. Read the comment docs.

Copy link
Member

@zainfathoni zainfathoni left a comment

Choose a reason for hiding this comment

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

LGTM 💯

Based on the exhaustive test on getKebabCase, now we're pretty confident that the function works as expected. Thanks a lot, @fncolon! 🙏

@zainfathoni zainfathoni dismissed ekamuktia’s stale review July 25, 2021 01:00

The requested changes have been implemented.

@zainfathoni zainfathoni enabled auto-merge July 25, 2021 01:00
@zainfathoni zainfathoni merged commit d77441d into kawalcovid19:main Jul 25, 2021
@fzn0x fzn0x deleted the patch-1 branch July 25, 2021 10:37
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.

Fix getKebabCase() implementation
3 participants