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

Update Left-side overlay positions #341

Merged
merged 8 commits into from
Apr 14, 2021
Merged

Update Left-side overlay positions #341

merged 8 commits into from
Apr 14, 2021

Conversation

cari-lynn
Copy link
Contributor

@cari-lynn cari-lynn commented Apr 6, 2021

This work is to update the position (file name and line number) of left side node when a document is overlayed. It is needed to report right side schema error messages, and also shows up when using the --output pos flag.

cari-lynn and others added 3 commits April 7, 2021 23:23
- need to build in a way to test line positions `template_test.go`
- run this test with `./hack/test-unit.sh -run TestYAMLTemplate
  TestYAMLTemplate.filetest=filetests/ytt-library/overlay/replace-key-position`
- extract stringifying evaluated template into separate function
@gcheadle-vmware gcheadle-vmware changed the title Update Right-side overlay positions Update Left-side overlay positions Apr 12, 2021
@gcheadle-vmware
Copy link
Contributor

We've found that in general, starlark structs have unknown positions. The unknown position now surfaces (using --output pos) when overlaying starlark structs. Thinking about the scope of this PR, we've decided that changes to the positions of starlark struct do not belong in this PR. If this is something the team wants to add, we should write another story to be picked up.

Starlark struct example:

#@ def return_starlark():
#@   return {"foo": "something"}
#@ end

cc: @cari-lynn

@gcheadle-vmware gcheadle-vmware marked this pull request as ready for review April 12, 2021 22:55
Copy link
Contributor

@pivotaljohn pivotaljohn left a comment

Choose a reason for hiding this comment

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

Reviewed together.

Notes:

  • in template_test.go look tearing off an interface for the AsBytes() function; interface{} means we can't commit to a given interface.
  • explore where the "Single Point Of Truth" could be for setting a value of a target node, given a replacing node.

- Change return and parameter type to a more strict interface

Signed-off-by: Steven Locke <[email protected]>
@cari-lynn
Copy link
Contributor Author

cari-lynn commented Apr 13, 2021

Addressed @jtigger 's comments above.

We created a Node interface method to set the position. We did not update references to where we are setting the position directly (ie: map.Position) and did not make the field private since we noticed there is not a consistent pattern for other fields currently. Happy to hear opinions if you have any regarding it.

- Preserving the Type and Metas fields on merge overlay operations is
  intentional in the anticipation that we may need them later for type
checking
- We did not change logic to preserve fields on overlay replace
  operations, but anticipate we may need to preserve some fields later
on.
Signed-off-by: John Ryan <[email protected]>
Copy link
Contributor

@pivotaljohn pivotaljohn left a comment

Choose a reason for hiding this comment

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

Reviewed synchronously.

In terms of "Single Point of Truth" — thanks for looking into this. Seems fine to let this ride for a bit as we watch the schema feature settling in. The comments left there I think will help future readers understand the current intent and what might change that implementation.

@pivotaljohn pivotaljohn merged commit 2f89b7c into develop Apr 14, 2021
@pivotaljohn pivotaljohn deleted the overlay-position branch April 14, 2021 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants