-
Notifications
You must be signed in to change notification settings - Fork 149
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
Constraint(s): introduce Equals()
and sort.Interface
#88
Conversation
Equals()
and sort.Interface
equal operator = '=' | ||
notEqual operator = '≠' | ||
greaterThan operator = '>' | ||
lessThan operator = '<' | ||
greaterThanEqual operator = '≥' | ||
lessThanEqual operator = '≤' | ||
pessimistic operator = '~' |
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.
Not sure if this is a good suggestion: but have you considered making of these strings, in case in a far future we want to be writing//rewriting these operators out to a 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.
I have considered that, but I concluded that exposing the operators would make them a public API and I wasn't sure what that API should really look like - e.g. whether equal
should be empty string or =
. i.e. there were some edge cases that I didn't want to decide, and so rune
makes it more obvious that this decision is yet to be made and it's more consciously not meant to be used directly as a public API, but purely for comparison.
You could argue we are already making it public API in the context of sort.Interface
, as turning runes to strings would likely affect order. However, I still think that displaying operators somewhere is a separate use case and so the potential future duplication (each operator having its own rune + string representation) is fine.
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.
Sounds good to me 🙂
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.
Super nice one, LGTM ! We have been doing these by had in some tests.
This is to allow us over in
terraform-ls
to detect configuration changes in Terraform core/provider version requirements more accurately and avoid triggering any unnecessary background jobs e.g. reloading schema version just due to whitespaces.The accuracy is rather a (positive) side effect of the implementation though, the main motivation was to have a single upstream equality function that we can call directly without having to maintain the logic downstream.
I guess it would be nice if
Constraints.String()
returned more "normalized" (sorted and whitespace-trimmed) constraints, but I'm afraid that ship has sailed and there would need to be a strong use case to weigh against the breaking change. This is also why as part of sorting I still intentionally kept the original order intact by copying - to avoid breaking changes.