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.

Sign up for GitHub

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

Jump to bottom

decoder: Add support for binary & unary operators #320

Merged
merged 5 commits into from
Sep 29, 2023
Merged

decoder: Add support for binary & unary operators #320

merged 5 commits into from
Sep 29, 2023

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Sep 25, 2023 •
edited
Loading

@radeksimko radeksimko added the enhancement New feature or request label Sep 25, 2023
@radeksimko radeksimko self-assigned this Sep 25, 2023
@radeksimko radeksimko force-pushed the f-expr-operators branch 4 times, most recently from 9a2fc64 to 6fb5908 Compare September 27, 2023 09:51
@radeksimko radeksimko mentioned this pull request Sep 27, 2023
Open
@radeksimko radeksimko marked this pull request as ready for review September 27, 2023 12:24
@radeksimko radeksimko requested a review from a team as a code owner September 27, 2023 12:24
dbanck
dbanck reviewed Sep 28, 2023
View reviewed changes
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

I'm not sure how extensively we want to test this. Since any expressions deal with references, functions and literal types, should we cover all three in all new tests?

decoder/expr_any_completion_test.go Show resolved Hide resolved
@radeksimko radeksimko mentioned this pull request Sep 29, 2023
Open
@radeksimko
Copy link
Member Author

I'm not sure how extensively we want to test this. Since any expressions deal with references, functions and literal types, should we cover all three in all new tests?

I would argue that some tests with functions are useful, as mentioned - and as added 😄 - specifically in completion, because incomplete configuration just brings so many more edge cases. 😭

However I'm not sure we'd actually gain much by adding tests to hover, semantic tokens or origins. The logic we have in place outside of completion should just treat any/all nested expression types the same (it's all hcl.Expression on the surface) and so one test with any expression type (such as traversal) should be sufficient to test that.

@radeksimko radeksimko requested a review from dbanck September 29, 2023 07:23
dbanck
dbanck approved these changes Sep 29, 2023
View reviewed changes
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

Thank you for adding the tests

@radeksimko radeksimko merged commit efd7ac8 into main Sep 29, 2023
@radeksimko radeksimko deleted the f-expr-operators branch September 29, 2023 08:08
@radeksimko radeksimko changed the title decoder: Implement completion for binary & unary operators decoder: Implement support for binary & unary operators Oct 4, 2023
@radeksimko radeksimko changed the title decoder: Implement support for binary & unary operators decoder: Add support for binary & unary operators Oct 4, 2023
@radeksimko radeksimko mentioned this pull request Oct 6, 2023
Merged
@radeksimko radeksimko mentioned this pull request Jan 4, 2024
Closed
@radeksimko radeksimko mentioned this pull request Jan 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbanck dbanck dbanck approved these changes

Assignees

@radeksimko radeksimko

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@radeksimko @dbanck