Add terraform_ordered_variables rule
#29
Conversation
lonegunmanb
changed the title
lonegunmanb
changed the title
terraform_ordered_variables rule
|
@wata727 I think this pr is ready, appreciate if you can give it a review. |
| return &lastRequired | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
nil causes panic. It should return an empty hcl.Range if there are no range.
There was a problem hiding this comment.
Hi, I left this nil by intention, please allow me to explain. this issueRange is called when we're sure that there is at least one violation inside these blocks, otherwise the caller method checkVariableOrder should return a nil as error instead. Let's say somehow we've a bug in this issueRange method that causes we cannot find any violation, which is wrong, an empty hcl.Range will confuse our user, make them wonder what this line 0 issue stands for. In this scenario I think maybe it's better to let it crash so we can receive a bug report that this method somehow has a bug. I'll leave this decision to you, if you prefer an empty hcl.Range, I'll be happy to change it in your way.
There was a problem hiding this comment.
Makes sense. In that case, I think calling panic explicitly is correct.
It is better to change the return type to hcl.Range instead of *hcl.Range, since it is not necessary to return nil.
There was a problem hiding this comment.
Done. Would you please give another review? Thanks.
There was a problem hiding this comment.
Sorry for the confusion. I would like to discuss about #22 (comment) before restarting the review.
lonegunmanb
force-pushed
the
variables-order
branch
from
9b3e7d8 to
47d7af4
Compare
|
Thank you for working on this. I'm sorry, but after consideration, this pull request will be closed for the time being. See discussion #22 (comment) for details. |
2 participants
This rule will check whether
variableblocks in a file are sorted by the following order:defaultargument) variables in alphabet orderdefaultargument) variables in alphabet order