[go: nahoru, domu]
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

firestore: firestore.Update should implement String() #8651

Open
wjkoh opened this issue Oct 5, 2023 · 5 comments
Open

firestore: firestore.Update should implement String() #8651

wjkoh opened this issue Oct 5, 2023 · 5 comments
Assignees
@bhshkh
Labels
api: firestore Issues related to the Firestore API. needs more info This issue needs more information from the customer to proceed. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@wjkoh
Copy link
wjkoh commented Oct 5, 2023 •
edited
Loading

I'm disappointed that firestore.Update does not implement String(), even though its underlying data type, DocumentTransform_FieldTransform, already does.

I have a public function in my project that returns firestore.Update and I want to add a unit test for it. Generally, I use the Google cmp library to compare what I want and what I got through cmp.Equal(). Unfortunately, firestore.Update has an unexported field, t, that contains all the information. I wish if we could implement Update.String() as func (u Update) String() string { return u.t.String() }. For now, I'm using my own stringfy function using reflect, but it is unsafe and inconvenient, as below.

func Stringfy(u *Update) string {
  rv := reflect.ValueOf(u.Value)
  rv = rv.FieldByName("t")
  dt := (*pb.DocumentTransform_FieldTransform)(rv.UnsafePointer())
  return fmt.Sprintf("Path: %s FieldPath: %s Value: %s", update.Path, update.FieldPath, dt.String())
}
@wjkoh wjkoh added the triage me I really want to be triaged. label Oct 5, 2023
@product-auto-label product-auto-label bot added the api: firestore Issues related to the Firestore API. label Oct 5, 2023
@noahdietz
Copy link
Contributor

Hi there, go-cmp has an option to allow comparison of unexported fields - AllowUnexported - so you don't need to write your own. Please close this issue if it solves the problem.

That said, if AllowUnexported doesn't solve the problem, keep this open and feel free to open a PR and contribute such a function yourself, it would be greatly appreciated.

Note: Please do not start a feature request with "I'm disappointed that X". This is the second issue I've read that leads with this language. It does not make anyone feel good or excited to solve a problem. I understand you may be frustrated by the lack of some functionality, but we are all busy developers like yourself, and shaming maintainers into implementing a feature isn't kind. Thank you for udnerstanding.

@noahdietz noahdietz added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. needs more info This issue needs more information from the customer to proceed. and removed triage me I really want to be triaged. labels Oct 16, 2023
@wjkoh
Copy link
Author
wjkoh commented Nov 11, 2023 •
edited
Loading

@noahdietz I chose to use "I'm disappointed ..." because the official template example on this repository was in that format. As seen in the screenshot below, why does it matter how the expression is worded if it accurately conveys the customer's feelings as long as it's not rude?

image

@noahdietz
Copy link
Contributor

I chose to use "I'm disappointed ..." because the official template example on this repository was in that format. As seen in the screenshot below,

Thanks for pointing that out, I will have it changed.

why does it matter how the expression is worded if it accurately conveys the customer's feelings as long as it's not rude?

Because we should never try, intentionally or otherwise, to make anyone feel bad. The folks that respond to this issue may not be the people that were involved in the code/issue in the first place. "I am disappointed" elicits a shame response in those that are "responsible" for the repository. It also implies that the code/repo in question is actively doing something wrong, which isn't always immediately obvious.

We can of course empathize with end user frustration and saying "Here is ABC potential issue/question. This is frustrating because XYZ" is entirely reasonable, but personally, I don't think that conveying such emotions is ever really constructive. When is an issue ever not frustrating, ya know?

go-cmp has an option to allow comparison of unexported fields - AllowUnexported - so you don't need to write your own. Please close this issue if it solves the problem.

That said, if AllowUnexported doesn't solve the problem, keep this open and feel free to open a PR and contribute such a function yourself, it would be greatly appreciated.

@wjkoh ^

@noahdietz noahdietz mentioned this issue Nov 27, 2023
Merged
@wjkoh
Copy link
Author
wjkoh commented Nov 28, 2023 •
edited
Loading

When I worked at Google, my team never viewed user reports that expressed their feelings as "shaming". Instead, we saw it as a valuable source of information. Is this Google's new official policy or just your personal opinion?

It appears to be unnecessary and not very productive. Consider how much time you and I are investing in something that doesn't benefit us. Please keep in mind that people who are submitting issues are likely customers paying for Firebase, and I think they should be allowed to express how they feel as long as they do so in a polite manner. We're not here for a lecture, we're here for a better product for our money.

Coming back to the issue, AllowUnexported is essentially the same as my workaround. A unit test could be influenced by internal changes. I wish there was a public method that could identify each update object or compare one with another, so we wouldn't need to worry when writing a unit test that compares it with a golden object. To reiterate, I already have a workaround and don't need another. I'm asking the team if adding a public method for this purpose is worth it.

@noahdietz
Copy link
Contributor

Is this Google's new official policy or just your personal opinion?

It's my personal opinion as one of the maintainers of this repo, with the agreement of other maintainers.

Consider how much time you and I are investing in something that doesn't benefit us.

I don't think this is a waste of time, I think this is valuable discourse on how we treat each other online. I was even able to improve (imo at least) our feature request issue template language as result: #9046.

We're not here for a lecture, we're here for a better product for our money.

And I'm here to help do that while helping maintain an intellectually/emotionally safe environment. What ever happened to just asking nicely e.g. "It would be great if X did Y, because Z"?

Sounds like this should be left open for @bhshkh or another firestore module maintainer to evaluate. Thanks for the explanation as to why using AllowUnexported isn't sufficient for your use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bhshkh bhshkh

Labels
api: firestore Issues related to the Firestore API. needs more info This issue needs more information from the customer to proceed. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@wjkoh @noahdietz @bhshkh and others