Fixes en/fr edge cases highlighted by @Bisaloo

epiverse-trace / numberize

Convert words to numbers in English, French and Spanish

https://epiverse-trace.github.io/numberize/

Other

4 stars 1 forks source link

Fixes en/fr edge cases highlighted by @Bisaloo #3

Closed bahadzie closed 5 months ago

Bisaloo commented 7 months ago

Hi @bahadzie, could you have a look at my review comments whenever convenient so we move forward with this PR? Thanks!

bahadzie commented 6 months ago

I apologise for not handling these earlier.

I got back to this repo because of some related work in {cleanepi} where they wanted to pass a vector instead of a character to numberize and that's when I noticed all this activity.

@Bisaloo @chartgerink what is your strategy for keeping up with repos that have received some comments/feedback?

bahadzie commented 6 months ago

Updated to handle vectors
If text param contains any non digit words, NA is returned

Bisaloo commented 6 months ago

Hi @bahadzie, thanks for this! Would you be able to split this PR to keep this one focused on the edge case and have another one on vectorization?

No worries if you are not sure how to do it, we can continue like this for this time and in the future:

discuss this kind of advanced git features
try to stick to the 1 feature = 1 PR rule

bahadzie commented 6 months ago

Hi @bahadzie, thanks for this! Would you be able to split this PR to keep this one focused on the edge case and have another one on vectorization?

Is this (SO: how-to-remove-commits-from-a-pull-request) what you mean?

I forked it to my github and tried it and is appears to work but I wanted to confirm with you first.

Bisaloo commented 6 months ago

Yes, that's correct.

Just make sure the commits you are removing from here are safely saved in another branch first. This can be achieved by creating another branch from this one (temp-fixes), such as vectorize, and then coming back to temp-fixes to follow the steps you listed.

bahadzie commented 6 months ago

Just make sure the commits you are removing from here are safely saved in another branch first. This can be achieved by creating another branch from this one (temp-fixes), such as vectorize, and then coming back to temp-fixes to follow the steps you listed.

@Bisaloo Done.

try to stick to the 1 feature = 1 PR rule

In the spirit of the above, should I separate the following into 2 PRs?

bug fix for text with leading or trailing whitespace
feature to deal with vectorized text e.g. c("one", "two", "three") returns c(1, 2, 3)

Bisaloo commented 6 months ago

In the spirit of the above, should I separate the following into 2 PRs?

bug fix for text with leading or trailing whitespace

feature to deal with vectorized text e.g. c("one", "two", "three") returns c(1, 2, 3)

Sure, if you can do this, that's ideal

bahadzie commented 6 months ago

Noted. Should I wait for this one to be merged into main so I can rebase of main to continue?

Bisaloo commented 6 months ago

No preference from my side. If you feel confident rebasing, then you can open now and rebase on main after this is integrated.