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

Remove example where type=null #13115

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove example where type=null #13115

wants to merge 1 commit into from

Conversation

lindhe
Copy link

@lindhe lindhe commented Jul 23, 2024

If type is always equal to null, it feels like an irrelevant example to bring up.

If type is always equal to null, it feels like an irrelevant example to bring up.
@lindhe lindhe requested a review from a team as a code owner July 23, 2024 08:54
@lindhe
Copy link
Author

lindhe commented Jul 23, 2024

Alternatively, we can change it to something more helpful. But if type=null in each example, I'm not sure what that contributes.

Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Hi @lindhe,

Thanks for the PR! Unfortunately I do not think we can accept it as-is, as the type in this case is indeed null, as in the null builder, not as "undefined".
I can completely understand that this is misleading though, and this deserves to be updated with more concrete examples than null to avoid that misunderstanding.

Do you want to take a jab at this change? I can take this on otherwise, please let me know how you'd prefer to proceed here.

Thanks!

@lindhe
Copy link
Author

lindhe commented Jul 31, 2024

[…] the type in this case is indeed null, as in the null builder, not as "undefined". I can completely understand that this is misleading though, and this deserves to be updated with more concrete examples than null to avoid that misunderstanding.

Ah, thanks for clarifying! That makes sense, I guess, just that it is pretty awkward to print it out on each line despite it never changing. Maybe just make a print like builder type {builder.type} on a separate line? Or perhaps there is some other builder we could use as another example…

Do you want to take a jab at this change? I can take this on otherwise, please let me know how you'd prefer to proceed here.

Thanks for asking. I'm usually happy to contribute, but I feel like I don't quite understand the purpose of this example so it's hard for me to make an improved example. No hard feelings if you ditch this PR and make a better example. 🙂

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