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

Support ignoring non-exported fields #81

Open
mitar opened this issue Oct 19, 2023 · 9 comments
Open

Support ignoring non-exported fields #81

mitar opened this issue Oct 19, 2023 · 9 comments

Comments

@mitar
Copy link

mitar commented Oct 19, 2023

If I have private struct with some exported fields (uppercase) and some not (lowercase) I would like that this linter complains only if I forget about an exported field. While other fields can be omitted (they are designed so).

@xobotyi
Copy link
Collaborator

xobotyi commented Oct 30, 2023

Then mark them as optional with field tags?

@mitar
Copy link
Author

mitar commented Oct 30, 2023

@xobotyi Sure. What I am asking here is that default would be for lowercase fields to be optional by default.

@xobotyi
Copy link
Collaborator

xobotyi commented Oct 30, 2023

It wont be default because

  1. it is a very rare case, the fact that you;re first asking since liner implemented 1.5 years ago
  2. will loosen linter's current behavior that is expected by everyone else

I will think about making the option to opt-out from pivate fields being mandatory

@mitar
Copy link
Author

mitar commented Oct 30, 2023

I will think about making the option to opt-out from pivate fields being mandatory

That. I do not want to change it for others. But having this option in golangci-lint would be awesome.

@navijation
Copy link
Contributor

Agree with @xobotyi. I'm not convinced private fields are inherently less important for callers witin the same package. What if you have a private field that needs to always be initialized within the package, e.g. a field that must be set inside a constructor? The analyzer offers no exhaustruct:"required" comment to override your default setting now.

Overall the value add seems small, and enabling this behavior is likely to make enforcement more inflexible / more complex to configure.

@mitar
Copy link
Author

mitar commented Nov 22, 2023

What if you have a private field that needs to always be initialized within the package, e.g. a field that must be set inside a constructor?

Then you create a New* function as a constructor for that type.

@navijation
Copy link
Contributor

@mitar If you require the New function you can't use the struct literal anyway. But if e.g. you have multiple constructors and always want the field to be set, now your choice of skipping enforcement bites you when you add a new private field and forget to initialize it in all constructors.

The key point is that there's nothing generally special about unexported fields that would suggest they shouldn't be set. Do you have some pattern in mind to motivate this special treatment?

@mitar
Copy link
Author

mitar commented Jan 3, 2024

(I do not want to go in circles here, so feel free to stop if you feel we are doing that.)

To me the motivation is really simple. I have a package with exported struct. That struct has exported (uppercase) fields. Main use case for that package and the struct is external use. This is why there are exported fields. And there is no need to initialize anything, so there is no New* function exported.

So external users of my package can use this struct and use it only be specifying exported fields. It works for them and the package and the struct is designed that it works only with exported fields set.

Private fields are just for caching/optimization/locks and are populated internally by methods of the struct as needed. I mean, sync.Mutex is designed that zero value just works, for example.

Now I run this linter on the code of this package. And linter complains because there struct initialization code which uses the struct in the same way as external user would. And I would like that the linter simulates how the external user sees the struct. If I add a public field that it requires from me that I add it to all test cases for example. And if I add a private field, I in fact want that I do not add it all around the package - because I want to make sure things work without that private field being initialized, because this is the contact I have in my package for external users of the package.

Do you have some pattern in mind to motivate this special treatment?

I think the pattern is that if the struct is designed that it is directly usable (without New*) for external users, then it can be used as such also internally. The fact that I export such struct is a contract/API I have for that struct. That setting only public fields is enough.

If I change the struct so that there is now a private field which has to be set, and add a New* function to initialize, I am changing the contract and breaking API anyway.

But you are right, it probably makes no sense that this is a package-level/linter-level setting. Probably what I am really asking for is that this should be a struct-level setting. So instead of me having to add a struct tag to all private fields, that I could mark the whole struct as having private fields optional. Maybe through some comment or something.

@navijation
Copy link
Contributor

@mitar

And if I add a private field, I in fact want that I do not add it all around the package - because I want to make sure things work without that private field being initialized, because this is the contact I have in my package for external users of the package.

That's a good point. I don't personally use many "plain-old data types" that have internal caching; those with complex state typically get constructors to enforce valid assignments, since I see the opposite case of initialization & validation requirements more often: in particular, maps and channels need to be assigned to avoid panics on writes. But the Go ecosystem probably differs from what I've written, given that the standard library usually accepts zero valued structs.

It does seem like more of a struct or package-level config, or perhaps even a global one if a required directive existed.

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

No branches or pull requests

3 participants