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

internal/schema: Replace TupleConsExpr with SetExpr #169

Merged
merged 2 commits into from
Dec 13, 2022

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Dec 12, 2022

Part of hashicorp/terraform-ls#496

The attributes in question will never be treated as data, so the data type doesn't really matter as much in this context. The main effect is hints in the completion and hover data.

Set seems most appropriate of all options (list, set, tuple), given that every element is of the same type (which rules out tuple) and order of elements doesn't matter (which rules out list).


The attributes in question will never be treated as data, so the data type doesn't really matter as much in this context. The main effect is hints in the completion and hover data.

Set seems most appropriate of all options (list, set, tuple), given that every element is of the same type (which rules out tuple) and order of elements doesn't matter (which rules out list).


UX Impact

(This implies hashicorp/hcl-lang#175 also in effect)

This has some relatively minor side effects in the completion and hover UX. One is mostly positive (experiments) and others are still okay (displaying singular instead of plural).

We can discuss whether the singular/plural thing is worth addressing and what the best version is there, but the PR is mainly meaning to solve the bug which is mis-rendering multiple constraints under collection types.

Also I am not sure if using more concrete description of a type, such as experiment is actually more helpful than just keyword. 🤷🏻

Before

Screenshot 2022-12-12 at 10 16 24
Screenshot 2022-12-12 at 10 16 45
Screenshot 2022-12-12 at 10 16 54
Screenshot 2022-12-12 at 10 17 06
Screenshot 2022-12-12 at 10 18 12

After

Screenshot 2022-12-12 at 10 26 23
Screenshot 2022-12-12 at 10 26 41
Screenshot 2022-12-12 at 10 26 52
Screenshot 2022-12-12 at 10 26 59
Screenshot 2022-12-12 at 10 27 36

The attributes in question will never be treated as data, so the data type doesn't really matter as much in this context. The main effect is hints in the completion and hover data.

Set seems most appropriate of all options (list, set, tuple), given that every element is of the same type (which rules out tuple) and order of elements doesn't matter (which rules out list).
@radeksimko radeksimko requested a review from a team December 12, 2022 10:18
@radeksimko radeksimko marked this pull request as ready for review December 12, 2022 10:28
@radeksimko radeksimko self-assigned this Dec 12, 2022
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.

Thanks for outlining the UX differences.

I think we don't need to change anything about the singular/plural wording. The expression wording is now more in line with the existing wording on other attributes/types.

CleanShot 2022-12-13 at 14 52 48@2x

CleanShot 2022-12-13 at 14 53 00@2x

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