Skip to content
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

implement Path.Equals and Path.HasPrefix #25

Merged
merged 1 commit into from
Jul 8, 2019
Merged

Conversation

jbardin
Copy link
Contributor

@jbardin jbardin commented Jul 5, 2019

Paths comparisons done with reflect.DeepEqual may erroneously return
false for numeric IndexSteps, due to the differences in the internal
values of the big.Float.

@jbardin jbardin changed the title implement Path.Equals implement Path.Matches Jul 5, 2019
@codecov
Copy link

codecov bot commented Jul 5, 2019

Codecov Report

Merging #25 into master will increase coverage by 0.06%.
The diff coverage is 82.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
+ Coverage   75.83%   75.89%   +0.06%     
==========================================
  Files          74       74              
  Lines        5727     5750      +23     
==========================================
+ Hits         4343     4364      +21     
- Misses        995      996       +1     
- Partials      389      390       +1
Impacted Files Coverage Δ
cty/path.go 69.23% <82.6%> (+4.52%) ⬆️
cty/value_ops.go 83.63% <0%> (+0.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fd39ad...66e2eb3. Read the comment docs.

@jbardin jbardin changed the title implement Path.Matches implement Path.Matches and Path.Equals Jul 5, 2019
@jbardin
Copy link
Contributor Author

jbardin commented Jul 5, 2019

Updated to change Equals to be a strict equality match.

Added Matches since it wasn't a strict equals comparison with the Unknown behavior. If we accept the Matches semantics rather than Equals, it wasn't a stretch to implement sub-path matching with wildcards. This allows an unknown value to match a value of the same type, and a shorter path to match against a longer one.

Copy link
Collaborator

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me! Thanks

I feel a little unsure about overloading unknown to mean a wildcard in Matches since I could see a calling application inadvertently placing an unknown in that position after evaluating an expression to get the key, without intending it to be treated as a wildcard. I don't really have a better idea, so if we need that right now then I suppose we could run with it, but if we can split the bug fix (actual numbers aren't matching) from the idea of wildcards (a new feature entirely, I think?) then I'd prefer that to see if we can find a less ambiguous way to deal with the wildcard idea... 🤔

@jbardin
Copy link
Contributor Author

jbardin commented Jul 5, 2019

Heh, I took the idea of the Unknown "wildcard" from the documentation ;)

the Key may be an unknown value, indicating that the step applies to any key of the given type

I did decide to split Equals from Matches, which separates the concepts a bit, but left it in since wildcards within paths are definitely a much requested feature within Terraform for depends_on and thought we could build it off this interface later.

@apparentlymart
Copy link
Collaborator

The path stuff here was extracted out of the otherwise-abandoned (and now split out until a separate repository) cty/diff package once I realized it was useful for other functionality, like Walk. I expect that comment was inherited from that other package that had some different goals.

Unless we're planning to implement wildcards right now, I'd prefer to leave this out so we can design it all together along with whatever other downstream features end up needed for it. The cty/diff package itself ended up abandoned because I tried to build it too soon without thinking through how the rest of the stack would work, and I worry this Matches function would end up the same.

Could we compromise and call this function HasPrefix for now, and only do the prefix-matching thing? Then when we revisit later we can still add Matches into here if it feels right in that context.

@jbardin
Copy link
Contributor Author

jbardin commented Jul 8, 2019

Equals and HasPrefix works for me. The unknown functionality was only because it seemed to be filling in a piece missing according to the docs. While that might be required to implement wildcard matches in TF depends_on, it's not necessary to fix the current equality issues.

@jbardin jbardin changed the title implement Path.Matches and Path.Equals implement Path.Equals and Path.HasPrefix Jul 8, 2019
Paths comparisons done with reflect.DeepEqual may erroneously return
false for numeric IndexSteps, due to the differences in the internal
values of the big.Float.

Add Path.Equals method to ensure all IndexSteps compare correctly.
Add Path.HasPrefix to test that one path is a prefix of another.
@jbardin jbardin merged commit 19588f9 into master Jul 8, 2019
@jbardin jbardin deleted the jbardin/path-equals branch July 8, 2019 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants