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

[feature] added rule for currency validation #178

Merged
merged 3 commits into from Aug 26, 2020

Conversation

ishan-srivastava
Copy link
Contributor

Regarding this feature request.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 99.787% when pulling cfbd715 on ishan-srivastava:master into 55d3e68 on icebob:master.

@erfanium erfanium requested review from erfanium and icebob July 18, 2020 04:44
@icebob
Copy link
Owner

icebob commented Jul 18, 2020

@ishan-srivastava thank you for your job on this PR.
I see this rule can cover the familiar currencies like, $, €, £ but not all.
In my country, the format of currency is: 1 234,56Ft
As you see, the thousand separator is space, the decimal separator is a comma and the sign at the end and two characters. Fully different, I know :)

image

Since you are using regex for testing, I think it would be hard to cover all types. The easier way can be that if we can set different checker regexp in the rule params. What do you think @ishan-srivastava @erfanium ?

@ishan-srivastava
Copy link
Contributor Author

Sure @icebob , that makes sense.
For what I can see is, that the validation logic can be changed by these params.

  1. Currency symbol string [ having options to make it optional, and case insensitive.]
  2. The currency is in prefix or postfix [default to prefix]
  3. String to allow between the Currency and digits in the format of
    a.) {character, minimum occurrences allowed, maximum occurrences allowed (leave blank for infinity) }
    b.) false if no String is allowed between the currency and digits.
  4. Character for decimal [deafults to a '.'].
  5. Character for thousand separator [deafults to a ',']

What we can also do is to extend the regex for the end-user to provide (in the schema), and that would override all the inbuilt validations the currency validator does, and would simply check the provided value against the regex provided by the user.

What do you think @icebob @erfanium ?

@icebob
Copy link
Owner

icebob commented Jul 18, 2020

Yeah, and please support negative currencies as well. I didn't see tests for negative values.

@erfanium
Copy link
Collaborator

erfanium commented Jul 18, 2020

What do you think @ishan-srivastava @erfanium ?

This is a good idea, ‌But I don't think we need to support all currencies, We can first support some important currencies and wait for user issues/PRs 🙂

This can be even more complicated, For example in Iran we have two major currencies: Rial and Toman Which can be converted into each other (10 Rial = 1 Toman). so maybe it is even better to have a separate rules for each currency.

@ishan-srivastava
Copy link
Contributor Author

ishan-srivastava commented Jul 18, 2020

@erfanium sure.
I think that in order to support more currencies, we would need to add more and more logic on top of existing logic, which won't be scalable.
Like you said : Adding a param that takes in a custom currency validator name, eg currency_rial / currency_hungarian_forint, sounds good to me.
The current logic would support all the currencies that have
a.) ',' as the thousand separator.
b.) '.' as decimal separator.
c.) Currency symbol is a single character, prefixed before the digits

@erfanium @icebob Should I go ahead and add the changes I suggested, or just simply support -ve values and update the PR?

@icebob
Copy link
Owner

icebob commented Jul 18, 2020

As @erfanium said, we don't need to support all currencies. So as you described, just add some property like thousand separator and decimal separator and a custom regexp pattern. So if I want to check Hungarian forint with this rule, I will define a custom regexp and set it to the rule.

Copy link
Owner

@icebob icebob left a comment

Choose a reason for hiding this comment

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

As @erfanium said, we don't need to support all currencies. So as you described, just add some property like thousand separator and decimal separator and a custom regexp pattern. So if I want to check Hungarian forint with this rule, I will define a custom regexp and set it to the rule.

@ishan-srivastava
Copy link
Contributor Author

Just add some property like thousand separator and decimal separator and a custom regexp pattern.

Sure, I'll add that in this week, day job has turned v hectic :p

@ishan-srivastava
Copy link
Contributor Author

@icebob I have added the following

  1. Feature to pass decimal separator
  2. Feature to pass thousand separator
  3. Feature to support negative currencies
  4. Feature to support custom regex.

Please have a look.

@erfanium
Copy link
Collaborator

erfanium commented Aug 24, 2020

@icebob What's the status of this PR?

@icebob
Copy link
Owner

icebob commented Aug 25, 2020

It's waiting for my review. I was busy but I will review it this week.

Copy link
Owner

@icebob icebob left a comment

Choose a reason for hiding this comment

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

Good job @ishan-srivastava, thanks!

@icebob icebob merged commit 94c67d8 into icebob:master Aug 26, 2020
@icebob
Copy link
Owner

icebob commented Aug 30, 2020

Released in 1.7.0

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.

None yet

4 participants