-
Notifications
You must be signed in to change notification settings - Fork 2
Use V2 of the UK VAT API and allow adding credentials #10
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
Conversation
|
|
||
| // Validate checks if the given VAT number exists and is active. If no error is returned, then it is. | ||
| func (s *ukVATService) Validate(vatNumber string) error { | ||
| func (s *ukVATService) Validate(vatNumber string, opts ValidatorOpts) error { |
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'd need to try this to be honest, but I'd suggest handling lookup service configuration, authentication and auth caching within the package. Sure it might take a bit longer but we're talking about major package version change anyway as we're either changing the Validate interface or the flow.
Personally I'd suggest to try providing additional "Configure" function if we want to keep the current structure. Flow would change to something like
vat.Configure(&ValidatorOptions{...})
or
vat.Configure([]LookupServiceInterface{NewUKLookupService{username, pass, etc.}, NewVEISLookupService})
// after that calling Validate would mean it goes through configured services
Validate(vatNumber) {
if vatNumber.HasPrefix(GB) {
if lookupService := vat.LookupServiceManager.get("GB") {
return lookupService.Validate(vatNumber)
} else {
return LookupServiceNotConfigured{}
}
}
}
alternatively ditch the single Validate function being exposed and provide a NewVat() with ability to configure validators, stop using the package directly. Initialise and pass where need instead.
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.
Discussed directly. We can look at the whole package design later on. For now we'll just keep it simple and get rid of the errors we're getting atm.
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 all sounds good, would probably need some sort of cache interface setup and what not. As @bakaat mentioned, we can use the changes in this PR as a "fix" for the UK issue in this version and we can implement those other types of changes in the next version (v4) of this package.
bakaat
left a comment
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.
lgtm
|
|
||
| // Validate checks if the given VAT number exists and is active. If no error is returned, then it is. | ||
| func (s *ukVATService) Validate(vatNumber string) error { | ||
| func (s *ukVATService) Validate(vatNumber string, opts ValidatorOpts) error { |
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.
Discussed directly. We can look at the whole package design later on. For now we'll just keep it simple and get rid of the errors we're getting atm.
No description provided.