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

attr: Added IsNull() and IsUnknown() methods to the Value interface type #194

Closed
wants to merge 2 commits into from

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Oct 1, 2021

Closes #193

@bflad bflad added enhancement New feature or request breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. types Issues and pull requests about our types abstraction and implementations. labels Oct 1, 2021
@bflad bflad requested a review from a team October 1, 2021 18:34
Copy link
Member

@kmoe kmoe left a comment

Choose a reason for hiding this comment

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

Needs review by @paddycarver but I'm on board with this as you know!

Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

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

I still think I prefer a tfsdk.ValueIsNull and tfsdk.ValueIsUnknown to reimplementing this on every single type.

I think asking type developers to implement the logic themselves creates opportunities for hard to debug situations where the value produces a null/unknown value when converted into a Terraform type, but reports that it is not null.

I also think it's extra work for type developers that doesn't need to be done and is just repeating yourself.

I recall you had some reasons for preferring not to do that. The one that comes to mind is that the tfsdk package is getting large and unwieldy, which I agree with, but I think can be mitigated by refactoring our package hierarchy.

Can you remind me what the other reasons you brought up were?

@paddycarver
Copy link
Contributor

Arguments for methods:

  • Discoverability: because standalone functions aren't tied to the attr.Value type, they're not surfaced by the language server, godoc, etc. as things you can do.

@paddycarver
Copy link
Contributor

We decided we wanted to wait on this momentarily until we can tackle #215, #172, and other refactoring issues, and refactor our package boundaries a little. Right now, it's a little hard to understand how large the discoverability tradeoff is going to be, just because we're guessing at what package architectures may be available in the future. I'm still hopeful we'll be able to have an attr package that wraps all this up neatly, but we can't today because we get import cycles. We'll return to this once we figure out what package boundaries we can draw after refactoring a bit.

@bflad
Copy link
Contributor Author

bflad commented Dec 13, 2021

I'm going to close this out for now given some other recent/upcoming changes. Can revisit this again in the future, if it makes sense.

@bflad bflad closed this Dec 13, 2021
@bflad bflad deleted the bflad-f-attr-Value-IsNull-IsUnknown branch December 13, 2021 17:59
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. enhancement New feature or request types Issues and pull requests about our types abstraction and implementations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

attr: Consider Adding IsNull() and IsUnknown() methods to Value
3 participants