-
Notifications
You must be signed in to change notification settings - Fork 44
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
Improve invalid package dimensions exceptions #35
Conversation
👍 |
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.
Do not forget to bump the version when other PR be merged.
correios/models/posting.py
Outdated
|
||
if service and service.max_weight and weight > service.max_weight: | ||
raise InvalidPackageWeightError("Max weight exceeded {!r}g (max. {!r}g)".format(weight, service.max_weight)) | ||
raise exceptions.InvalidPackageWeightError( |
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 believe the correct exception here is InvalidMaxPackageWeightError
. Isn't it?
I'll will change the minimum validations to |
Why ? Isn't d=0, invalid ? |
|
I rewrote the whole PR to allow dimensions below the minimum in the packages but maintaining the minimum dimensions for volume and weight calculation. |
correios/models/posting.py
Outdated
if weight <= 0: | ||
raise exceptions.InvalidMinPackageWeightError("Invalid weight {!r}g".format(weight)) | ||
|
||
if self.service and weight > self.service.max_weight: |
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.
Two things here:
- As I could see, it's possible to exist a Package without a service. The business rules allow a Package without service? Seems weird.
- I think the exception message could be improved putting the service name on it.
Max weight exceeded [...] for service xpto
. If you accept it, there's another equal message invalidate()
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.
As I could see, it's possible to exist a Package without a service. The business rules allow a Package without service? Seems weird.
A Package is just a package with their type and dimensions and weight. So we can have a package before the Service definition (we can choose the service for this package after the package already exists). We can also choose the correct service as a consequence of the package weight.
There is a modeling issue: package doesn't need a service
property/attribute. It must delegate the weight validation to the ShippingLabel. But this is a hard refactor to do and we don't need it for now.
I think the exception message could be improved putting the service name on it. Max weight exceeded [...] for service xpto. If you accept it, there's another equal message in validate()
👍
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.
Só um pequeno comentário, o resto tá 👍
correios/models/posting.py
Outdated
self.sequence = sequence | ||
self.service = service | ||
|
||
@property | ||
def volumetric_weight(self): | ||
def width(self): |
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.
nessas properties não vão type hints?
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.
esquecimento :P
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.
feito
Improve invalid package dimensions exceptions
Improve package dimension validation to raise different exceptions for minimum vs. maximum invalid dimensions.
I'll wait for #34 merge and then bump minor version to make a release of this PR.