-
Notifications
You must be signed in to change notification settings - Fork 25
compliance/sancation_list_update_centralization #45
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
base: master
Are you sure you want to change the base?
compliance/sancation_list_update_centralization #45
Conversation
d28123d
to
3db7903
Compare
@@ -0,0 +1,230 @@ | |||
package Data::Validate::Sanctions::Redis; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest naming it Data::Validate::Sanctions::Storage
instead of Data::Validate::Sanctions::Redis
. Redis
is just a system that we are using, we may change this in future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module is exclusively created for redis storage. It relies on hmget
and hmset
calls and cannot be written without references to Redis. A better alternative IMHO is to use the parent class Data::Validate::Sactions
as a Factory that can create an object of this subclass with a code like this:
my $validator = Data::Validate:::Sanctions->new(storage => 'redis`, connection => $redis);
…hub.com:mat-fs/perl-Data-Validate-Sanctions into compliance/sancation_list_update_centralization
.circleci/config.yml
Outdated
- run: | ||
name: Install Redis server | ||
command: | | ||
apt-get install -y redis && apt-get install -y redis-server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you need install only redis-server
and if you want to install both , you can combine them:
apt-get install -y redis && apt-get install -y redis-server | |
apt-get install -y redis-server redis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Changes
Outdated
@@ -1,6 +1,9 @@ | |||
Revision history for Data-Validate-Sanctions | |||
|
|||
{{$NEXT}} | |||
0.14 2022-09-13 13:55:00 CST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can & should be removed. dzil release
will add it automatically
@@ -87,6 +103,7 @@ sub set_sanction_file { ## no critic (RequireArgUnpacking) | |||
} | |||
|
|||
sub get_sanction_file { | |||
$sanction_file //= _default_sanction_file(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be better to use state
here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in some cases, the state declaration does not apply because we are using the internal scope of the variable in the set_sanction_file
and the get_sanction_file
subroutines.
https://redmine.deriv.cloud/issues/70209